LUCENE-4876: don't clone IndexWriterConfig by IndexWriter; prevent config from being shared with multiple writers

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1507704 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -96,6 +96,11 @@ API Changes
 * LUCENE-5129: CategoryAssociationsContainer no longer supports null
   association values for categories. If you want to index categories without
   associations, you should add them using FacetFields. (Shai Erera)
+
+* LUCENE-4876: IndexWriter no longer clones the given IndexWriterConfig. If you
+  need to use the same config more than once, e.g. when sharing between multiple 
+  writers, make sure to clone it before passing to each writer.
+  (Shai Erera, Mike McCandless)
   
 Optimizations
 

lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java

@@ -147,7 +147,10 @@ abstract class DocumentsWriterPerThreadPool implements Cloneable {
   @Override
   public DocumentsWriterPerThreadPool clone() {
     // We should only be cloned before being used:
-    assert numThreadStatesActive == 0;
+    if (numThreadStatesActive != 0) {
+      throw new IllegalStateException("clone this object before it is used!");
+    }
+    
     DocumentsWriterPerThreadPool clone;
     try {
       clone = (DocumentsWriterPerThreadPool) super.clone();

lucene/core/src/java/org/apache/lucene/index/IndexWriter.java

@@ -630,15 +630,13 @@ public class IndexWriter implements Closeable, TwoPhaseCommit {
 
   /**
    * Constructs a new IndexWriter per the settings given in <code>conf</code>.
-   * Note that the passed in {@link IndexWriterConfig} is
+   * If you want to make "live" changes to this writer instance, use
-   * privately cloned, which, in-turn, clones the
+   * {@link #getConfig()}.
-   * {@link IndexWriterConfig#getFlushPolicy() flush policy},
+   * 
-   * {@link IndexWriterConfig#getIndexDeletionPolicy() deletion policy},
-   * {@link IndexWriterConfig#getMergePolicy() merge policy},
-   * and {@link IndexWriterConfig#getMergeScheduler() merge scheduler}.
-   * If you need to make subsequent "live"
-   * changes to the configuration use {@link #getConfig}.
    * <p>
+   * <b>NOTE:</b> after ths writer is created, the given configuration instance
+   * cannot be passed to another writer. If you intend to do so, you should
+   * {@link IndexWriterConfig#clone() clone} it beforehand.
    * 
    * @param d
    *          the index directory. The index is either created or appended
@@ -653,7 +651,8 @@ public class IndexWriter implements Closeable, TwoPhaseCommit {
    *           IO error
    */
   public IndexWriter(Directory d, IndexWriterConfig conf) throws IOException {
-    config = new LiveIndexWriterConfig(conf.clone());
+    conf.setIndexWriter(this); // prevent reuse by other instances
+    config = new LiveIndexWriterConfig(conf);
     directory = d;
     analyzer = config.getAnalyzer();
     infoStream = config.getInfoStream();

lucene/core/src/java/org/apache/lucene/index/IndexWriterConfig.java

@@ -26,6 +26,8 @@ import org.apache.lucene.index.IndexWriter.IndexReaderWarmer;
 import org.apache.lucene.search.similarities.Similarity;
 import org.apache.lucene.util.InfoStream;
 import org.apache.lucene.util.PrintStreamInfoStream;
+import org.apache.lucene.util.SetOnce;
+import org.apache.lucene.util.SetOnce.AlreadySetException;
 import org.apache.lucene.util.Version;
 
 /**
@@ -132,6 +134,21 @@ public final class IndexWriterConfig extends LiveIndexWriterConfig implements Cl
     return WRITE_LOCK_TIMEOUT;
   }
 
+  // indicates whether this config instance is already attached to a writer.
+  // not final so that it can be cloned properly.
+  private SetOnce<IndexWriter> writer = new SetOnce<IndexWriter>();
+  
+  /**
+   * Sets the {@link IndexWriter} this config is attached to.
+   * 
+   * @throws AlreadySetException
+   *           if this config is already attached to a writer.
+   */
+  IndexWriterConfig setIndexWriter(IndexWriter writer) {
+    this.writer.set(writer);
+    return this;
+  }
+  
   /**
    * Creates a new config that with defaults that match the specified
    * {@link Version} as well as the default {@link
@@ -152,6 +169,8 @@ public final class IndexWriterConfig extends LiveIndexWriterConfig implements Cl
     try {
       IndexWriterConfig clone = (IndexWriterConfig) super.clone();
       
+      clone.writer = writer.clone();
+      
       // Mostly shallow clone, but do a deepish clone of
       // certain objects that have state that cannot be shared
       // across IW instances:
@@ -545,8 +564,16 @@ public final class IndexWriterConfig extends LiveIndexWriterConfig implements Cl
     return (IndexWriterConfig) super.setTermIndexInterval(interval);
   }
   
+  @Override
   public IndexWriterConfig setUseCompoundFile(boolean useCompoundFile) {
     return (IndexWriterConfig) super.setUseCompoundFile(useCompoundFile);
   }
 
+  @Override
+  public String toString() {
+    StringBuilder sb = new StringBuilder(super.toString());
+    sb.append("writer=").append(writer).append("\n");
+    return sb.toString();
+  }
+  
 }

lucene/core/src/java/org/apache/lucene/util/SetOnce.java

@@ -28,7 +28,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
  *
  * @lucene.experimental
  */
-public final class SetOnce<T> {
+public final class SetOnce<T> implements Cloneable {
 
   /** Thrown when {@link SetOnce#set(Object)} is called more than once. */
   public static final class AlreadySetException extends IllegalStateException {
@@ -74,4 +74,10 @@ public final class SetOnce<T> {
   public final T get() {
     return obj;
   }
+  
+  @Override
+  public SetOnce<T> clone() {
+    return obj == null ? new SetOnce<T>() : new SetOnce<T>(obj);
+  }
+  
 }

lucene/core/src/test/org/apache/lucene/codecs/lucene41/TestBlockPostingsFormat2.java

@@ -46,7 +46,7 @@ public class TestBlockPostingsFormat2 extends LuceneTestCase {
     dir = newFSDirectory(_TestUtil.getTempDir("testDFBlockSize"));
     iwc = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
     iwc.setCodec(_TestUtil.alwaysPostingsFormat(new Lucene41PostingsFormat()));
-    iw = new RandomIndexWriter(random(), dir, iwc);
+    iw = new RandomIndexWriter(random(), dir, iwc.clone());
     iw.setDoRandomForceMerge(false); // we will ourselves
   }
   
@@ -55,7 +55,7 @@ public class TestBlockPostingsFormat2 extends LuceneTestCase {
     iw.close();
     _TestUtil.checkIndex(dir); // for some extra coverage, checkIndex before we forceMerge
     iwc.setOpenMode(OpenMode.APPEND);
-    IndexWriter iw = new IndexWriter(dir, iwc);
+    IndexWriter iw = new IndexWriter(dir, iwc.clone());
     iw.forceMerge(1);
     iw.close();
     dir.close(); // just force a checkindex for now

lucene/core/src/test/org/apache/lucene/codecs/lucene41/TestBlockPostingsFormat3.java

@@ -86,7 +86,7 @@ public class TestBlockPostingsFormat3 extends LuceneTestCase {
     iwc.setCodec(_TestUtil.alwaysPostingsFormat(new Lucene41PostingsFormat())); 
     // TODO we could actually add more fields implemented with different PFs
     // or, just put this test into the usual rotation?
-    RandomIndexWriter iw = new RandomIndexWriter(random(), dir, iwc);
+    RandomIndexWriter iw = new RandomIndexWriter(random(), dir, iwc.clone());
     Document doc = new Document();
     FieldType docsOnlyType = new FieldType(TextField.TYPE_NOT_STORED);
     // turn this on for a cross-check
@@ -138,7 +138,7 @@ public class TestBlockPostingsFormat3 extends LuceneTestCase {
     verify(dir);
     _TestUtil.checkIndex(dir); // for some extra coverage, checkIndex before we forceMerge
     iwc.setOpenMode(OpenMode.APPEND);
-    IndexWriter iw2 = new IndexWriter(dir, iwc);
+    IndexWriter iw2 = new IndexWriter(dir, iwc.clone());
     iw2.forceMerge(1);
     iw2.close();
     verify(dir);

lucene/core/src/test/org/apache/lucene/index/TestDocValuesIndexing.java

@@ -558,13 +558,13 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testIllegalTypeChangeAcrossSegments() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
 
-    writer = new IndexWriter(dir, conf);
+    writer = new IndexWriter(dir, conf.clone());
     doc = new Document();
     doc.add(new SortedDocValuesField("dv", new BytesRef("foo")));
     try {
@@ -580,13 +580,13 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testTypeChangeAfterCloseAndDeleteAll() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
 
-    writer = new IndexWriter(dir, conf);
+    writer = new IndexWriter(dir, conf.clone());
     writer.deleteAll();
     doc = new Document();
     doc.add(new SortedDocValuesField("dv", new BytesRef("foo")));
@@ -629,13 +629,13 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testTypeChangeAfterOpenCreate() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
     conf.setOpenMode(IndexWriterConfig.OpenMode.CREATE);
-    writer = new IndexWriter(dir, conf);
+    writer = new IndexWriter(dir, conf.clone());
     doc = new Document();
     doc.add(new SortedDocValuesField("dv", new BytesRef("foo")));
     writer.addDocument(doc);
@@ -646,14 +646,14 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testTypeChangeViaAddIndexes() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
 
     Directory dir2 = newDirectory();
-    writer = new IndexWriter(dir2, conf);
+    writer = new IndexWriter(dir2, conf.clone());
     doc = new Document();
     doc.add(new SortedDocValuesField("dv", new BytesRef("foo")));
     writer.addDocument(doc);
@@ -672,14 +672,14 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testTypeChangeViaAddIndexesIR() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
 
     Directory dir2 = newDirectory();
-    writer = new IndexWriter(dir2, conf);
+    writer = new IndexWriter(dir2, conf.clone());
     doc = new Document();
     doc.add(new SortedDocValuesField("dv", new BytesRef("foo")));
     writer.addDocument(doc);
@@ -700,14 +700,14 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testTypeChangeViaAddIndexes2() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
 
     Directory dir2 = newDirectory();
-    writer = new IndexWriter(dir2, conf);
+    writer = new IndexWriter(dir2, conf.clone());
     writer.addIndexes(dir);
     doc = new Document();
     doc.add(new SortedDocValuesField("dv", new BytesRef("foo")));
@@ -725,14 +725,14 @@ public class TestDocValuesIndexing extends LuceneTestCase {
   public void testTypeChangeViaAddIndexesIR2() throws Exception {
     Directory dir = newDirectory();
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
-    IndexWriter writer = new IndexWriter(dir, conf);
+    IndexWriter writer = new IndexWriter(dir, conf.clone());
     Document doc = new Document();
     doc.add(new NumericDocValuesField("dv", 0L));
     writer.addDocument(doc);
     writer.close();
 
     Directory dir2 = newDirectory();
-    writer = new IndexWriter(dir2, conf);
+    writer = new IndexWriter(dir2, conf.clone());
     IndexReader[] readers = new IndexReader[] {DirectoryReader.open(dir)};
     writer.addIndexes(readers);
     readers[0].close();

lucene/core/src/test/org/apache/lucene/index/TestIndexWriterConfig.java

@@ -37,6 +37,7 @@ import org.apache.lucene.search.similarities.DefaultSimilarity;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.util.InfoStream;
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SetOnce.AlreadySetException;
 import org.junit.Test;
 
 public class TestIndexWriterConfig extends LuceneTestCase {
@@ -145,18 +146,30 @@ public class TestIndexWriterConfig extends LuceneTestCase {
   @Test
   public void testReuse() throws Exception {
     Directory dir = newDirectory();
-    // test that if the same IWC is reused across two IWs, it is cloned by each.
+    // test that IWC cannot be reused across two IWs
     IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, null);
-    RandomIndexWriter iw = new RandomIndexWriter(random(), dir, conf);
+    new RandomIndexWriter(random(), dir, conf).close();
-    LiveIndexWriterConfig liveConf1 = iw.w.getConfig();
+
-    iw.close();
+    // this should fail
+    try {
+      assertNotNull(new RandomIndexWriter(random(), dir, conf));
+      fail("should have hit AlreadySetException");
+    } catch (AlreadySetException e) {
+      // expected
+    }
+
+    // also cloning it won't help, after it has been used already
+    try {
+      assertNotNull(new RandomIndexWriter(random(), dir, conf.clone()));
+      fail("should have hit AlreadySetException");
+    } catch (AlreadySetException e) {
+      // expected
+    }
     
-    iw = new RandomIndexWriter(random(), dir, conf);
+    // if it's cloned in advance, it should be ok
-    LiveIndexWriterConfig liveConf2 = iw.w.getConfig();
+    conf = newIndexWriterConfig(TEST_VERSION_CURRENT, null);
-    iw.close();
+    new RandomIndexWriter(random(), dir, conf.clone()).close();
-    
+    new RandomIndexWriter(random(), dir, conf.clone()).close();
-    // LiveIndexWriterConfig's "copy" constructor doesn't clone objects.
-    assertNotSame("IndexWriterConfig should have been cloned", liveConf1.getMergePolicy(), liveConf2.getMergePolicy());
     
     dir.close();
   }

lucene/core/src/test/org/apache/lucene/index/TestIndexWriterDelete.java

@@ -1152,7 +1152,7 @@ public class TestIndexWriterDelete extends LuceneTestCase {
     Directory dir = newDirectory();
     IndexWriterConfig iwc = new IndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
     iwc.setMaxBufferedDocs(2);
-    IndexWriter w = new IndexWriter(dir, iwc);
+    IndexWriter w = new IndexWriter(dir, iwc.clone());
     Document doc = new Document();
     doc.add(newField("field", "0", StringField.TYPE_NOT_STORED));
     w.addDocument(doc);
@@ -1177,7 +1177,7 @@ public class TestIndexWriterDelete extends LuceneTestCase {
 
     // Segment should have deletions:
     assertTrue(s.contains("has deletions"));
-    w = new IndexWriter(dir, iwc);
+    w = new IndexWriter(dir, iwc.clone());
     w.forceMerge(1);
     w.close();
 

lucene/core/src/test/org/apache/lucene/index/TestSnapshotDeletionPolicy.java

@@ -42,7 +42,7 @@ public class TestSnapshotDeletionPolicy extends LuceneTestCase {
   public static final String INDEX_PATH = "test.snapshots";
   
   protected IndexWriterConfig getConfig(Random random, IndexDeletionPolicy dp) {
-    IndexWriterConfig conf = newIndexWriterConfig( TEST_VERSION_CURRENT, new MockAnalyzer(random));
+    IndexWriterConfig conf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random));
     if (dp != null) {
       conf.setIndexDeletionPolicy(dp);
     }
@@ -323,8 +323,8 @@ public class TestSnapshotDeletionPolicy extends LuceneTestCase {
     int numSnapshots = 2;
     Directory dir = newDirectory();
 
-    IndexWriter writer = new IndexWriter(dir, getConfig(random(), getDeletionPolicy()));
+    SnapshotDeletionPolicy sdp = getDeletionPolicy();
-    SnapshotDeletionPolicy sdp = (SnapshotDeletionPolicy) writer.getConfig().getIndexDeletionPolicy();
+    IndexWriter writer = new IndexWriter(dir, getConfig(random(), sdp));
     prepareIndexAndSnapshots(sdp, writer, numSnapshots);
     writer.close();
 
@@ -333,8 +333,7 @@ public class TestSnapshotDeletionPolicy extends LuceneTestCase {
     // this does the actual rollback
     writer.commit();
     writer.deleteUnusedFiles();
-    //sdp = (SnapshotDeletionPolicy) writer.getConfig().getIndexDeletionPolicy();
+    assertSnapshotExists(dir, sdp, numSnapshots - 1, false);
-    assertSnapshotExists(dir, sdp, numSnapshots - 1, true);
     writer.close();
 
     // but 'snapshot1' files will still exist (need to release snapshot before they can be deleted).

lucene/replicator/src/java/org/apache/lucene/replicator/IndexAndTaxonomyRevision.java

@@ -78,15 +78,14 @@ public class IndexAndTaxonomyRevision implements Revision {
     @Override
     protected IndexWriterConfig createIndexWriterConfig(OpenMode openMode) {
       IndexWriterConfig conf = super.createIndexWriterConfig(openMode);
-      conf.setIndexDeletionPolicy(new SnapshotDeletionPolicy(conf.getIndexDeletionPolicy()));
+      sdp = new SnapshotDeletionPolicy(conf.getIndexDeletionPolicy());
+      conf.setIndexDeletionPolicy(sdp);
       return conf;
     }
     
     @Override
     protected IndexWriter openIndexWriter(Directory directory, IndexWriterConfig config) throws IOException {
       writer = super.openIndexWriter(directory, config);
-      // must set it here because IndexWriter clones the config
-      sdp = (SnapshotDeletionPolicy) writer.getConfig().getIndexDeletionPolicy();
       return writer;
     }
     

lucene/test-framework/src/java/org/apache/lucene/index/BaseStoredFieldsFormatTestCase.java

@@ -509,7 +509,7 @@ public abstract class BaseStoredFieldsFormatTestCase extends LuceneTestCase {
     Directory dir = newDirectory();
     IndexWriterConfig iwConf = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()));
     iwConf.setMaxBufferedDocs(RandomInts.randomIntBetween(random(), 2, 30));
-    RandomIndexWriter iw = new RandomIndexWriter(random(), dir, iwConf);
+    RandomIndexWriter iw = new RandomIndexWriter(random(), dir, iwConf.clone());
     
     final int docCount = atLeast(200);
     final byte[][][] data = new byte [docCount][][];
@@ -548,7 +548,7 @@ public abstract class BaseStoredFieldsFormatTestCase extends LuceneTestCase {
         } else {
           iwConf.setCodec(otherCodec);
         }
-        iw = new RandomIndexWriter(random(), dir, iwConf);
+        iw = new RandomIndexWriter(random(), dir, iwConf.clone());
       }
     }