Index: lucene/CHANGES.txt =================================================================== --- lucene/CHANGES.txt (revision 1060338) +++ lucene/CHANGES.txt (working copy) @@ -323,6 +323,9 @@ * LUCENE-2860: Fixed SegmentInfo.sizeInBytes to factor includeDocStores when it decides whether to return the cached computed size or not. (Shai Erera) +* LUCENE-2584: SegmentInfo.files() could hit ConcurrentModificationException if + called by multiple threads. (Alexander Kanarsky via Shai Erera) + New features * LUCENE-2128: Parallelized fetching document frequencies during weight Index: lucene/src/test/org/apache/lucene/index/TestSegmentInfo.java =================================================================== --- lucene/src/test/org/apache/lucene/index/TestSegmentInfo.java (revision 1060338) +++ lucene/src/test/org/apache/lucene/index/TestSegmentInfo.java (working copy) @@ -1,12 +1,17 @@ package org.apache.lucene.index; +import java.io.IOException; +import java.util.Iterator; + import org.apache.lucene.analysis.MockAnalyzer; import org.apache.lucene.document.Document; import org.apache.lucene.document.Field; import org.apache.lucene.document.Field.Index; import org.apache.lucene.document.Field.Store; +import org.apache.lucene.document.Field.TermVector; import org.apache.lucene.store.Directory; import org.apache.lucene.util.LuceneTestCase; +import org.apache.lucene.util._TestUtil; /** * Licensed to the Apache Software Foundation (ASF) under one or more @@ -45,4 +50,41 @@ dir.close(); } + // LUCENE-2584: calling files() by multiple threads could lead to ConcurrentModificationException + public void testFilesConcurrency() throws Exception { + Directory dir = newDirectory(); + // Create many files + IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer()); + IndexWriter writer = new IndexWriter(dir, conf); + Document doc = new Document(); + doc.add(new Field("a", "b", Store.YES, Index.ANALYZED, TermVector.YES)); + writer.addDocument(doc); + writer.close(); + + SegmentInfos sis = new SegmentInfos(); + sis.read(dir); + final SegmentInfo si = sis.info(0); + Thread[] threads = new Thread[_TestUtil.nextInt(random, 2, 5)]; + for (int i = 0; i < threads.length; i++) { + threads[i] = new Thread() { + @Override + public void run() { + try { + // Verify that files() does not throw an exception and that the + // iteration afterwards succeeds. + Iterator iter = si.files().iterator(); + while (iter.hasNext()) iter.next(); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + }; + } + + for (Thread t : threads) t.start(); + for (Thread t : threads) t.join(); + + dir.close(); + } + } Index: lucene/src/java/org/apache/lucene/index/SegmentInfo.java =================================================================== --- lucene/src/java/org/apache/lucene/index/SegmentInfo.java (revision 1060338) +++ lucene/src/java/org/apache/lucene/index/SegmentInfo.java (working copy) @@ -22,11 +22,13 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.util.BitVector; import java.io.IOException; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.HashMap; import java.util.ArrayList; import java.util.Collections; +import java.util.Set; /** * Information about a segment such as it's name, directory, and files related @@ -604,7 +606,7 @@ return hasProx; } - private void addIfExists(List files, String fileName) throws IOException { + private void addIfExists(Set files, String fileName) throws IOException { if (dir.fileExists(fileName)) files.add(fileName); } @@ -622,15 +624,15 @@ return files; } - files = new ArrayList(); + HashSet filesSet = new HashSet(); boolean useCompoundFile = getUseCompoundFile(); if (useCompoundFile) { - files.add(IndexFileNames.segmentFileName(name, IndexFileNames.COMPOUND_FILE_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(name, IndexFileNames.COMPOUND_FILE_EXTENSION)); } else { for (String ext : IndexFileNames.NON_STORE_INDEX_EXTENSIONS) - addIfExists(files, IndexFileNames.segmentFileName(name, ext)); + addIfExists(filesSet, IndexFileNames.segmentFileName(name, ext)); } if (docStoreOffset != -1) { @@ -638,29 +640,29 @@ // vectors) with other segments assert docStoreSegment != null; if (docStoreIsCompoundFile) { - files.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.COMPOUND_FILE_STORE_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.COMPOUND_FILE_STORE_EXTENSION)); } else { - files.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.FIELDS_INDEX_EXTENSION)); - files.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.FIELDS_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.FIELDS_INDEX_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.FIELDS_EXTENSION)); if (hasVectors) { - files.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.VECTORS_INDEX_EXTENSION)); - files.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.VECTORS_DOCUMENTS_EXTENSION)); - files.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.VECTORS_FIELDS_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.VECTORS_INDEX_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.VECTORS_DOCUMENTS_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(docStoreSegment, IndexFileNames.VECTORS_FIELDS_EXTENSION)); } } } else if (!useCompoundFile) { - files.add(IndexFileNames.segmentFileName(name, IndexFileNames.FIELDS_INDEX_EXTENSION)); - files.add(IndexFileNames.segmentFileName(name, IndexFileNames.FIELDS_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(name, IndexFileNames.FIELDS_INDEX_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(name, IndexFileNames.FIELDS_EXTENSION)); if (hasVectors) { - files.add(IndexFileNames.segmentFileName(name, IndexFileNames.VECTORS_INDEX_EXTENSION)); - files.add(IndexFileNames.segmentFileName(name, IndexFileNames.VECTORS_DOCUMENTS_EXTENSION)); - files.add(IndexFileNames.segmentFileName(name, IndexFileNames.VECTORS_FIELDS_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(name, IndexFileNames.VECTORS_INDEX_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(name, IndexFileNames.VECTORS_DOCUMENTS_EXTENSION)); + filesSet.add(IndexFileNames.segmentFileName(name, IndexFileNames.VECTORS_FIELDS_EXTENSION)); } } String delFileName = IndexFileNames.fileNameFromGeneration(name, IndexFileNames.DELETES_EXTENSION, delGen); if (delFileName != null && (delGen >= YES || dir.fileExists(delFileName))) { - files.add(delFileName); + filesSet.add(delFileName); } // Careful logic for norms files @@ -669,14 +671,14 @@ long gen = normGen[i]; if (gen >= YES) { // Definitely a separate norm file, with generation: - files.add(IndexFileNames.fileNameFromGeneration(name, IndexFileNames.SEPARATE_NORMS_EXTENSION + i, gen)); + filesSet.add(IndexFileNames.fileNameFromGeneration(name, IndexFileNames.SEPARATE_NORMS_EXTENSION + i, gen)); } else if (NO == gen) { // No separate norms but maybe plain norms // in the non compound file case: if (!hasSingleNormFile && !useCompoundFile) { String fileName = IndexFileNames.segmentFileName(name, IndexFileNames.PLAIN_NORMS_EXTENSION + i); if (dir.fileExists(fileName)) { - files.add(fileName); + filesSet.add(fileName); } } } else if (CHECK_DIR == gen) { @@ -688,7 +690,7 @@ fileName = IndexFileNames.segmentFileName(name, IndexFileNames.PLAIN_NORMS_EXTENSION + i); } if (fileName != null && dir.fileExists(fileName)) { - files.add(fileName); + filesSet.add(fileName); } } } @@ -706,11 +708,11 @@ for(int i=0;i prefixLength && Character.isDigit(fileName.charAt(prefixLength)) && fileName.startsWith(prefix)) { - files.add(fileName); + filesSet.add(fileName); } } } - return files; + return files = new ArrayList(filesSet); } /* Called whenever any change is made that affects which