stevenschlansker opened a new issue, #11913:
URL: https://github.com/apache/lucene/issues/11913

   ### Description
   
   In the lucene-replicator module, the PrimaryNode does some initialization 
work in the constructor. It starts with an IndexWriter provided by the 
application author. At line 92:
   
   ```
   writer.getConfig().setMergedSegmentWarmer(new 
PreCopyMergedSegmentWarmer(this));
   ```
   
   In this line, the IndexWriter is mutated to indirectly reference 
`PrimaryNode.this` before the constructor initialization has completed. At this 
point, important fields are not set yet (`mgr` for example is not set), and 
further initialization is not done carefully with regards to thread safety 
(`mgr` is not volatile, for example).
   
   Meanwhile, a background thread calls `IndexWriter.commit()`, not realizing 
the `PrimaryNode` is not yet initialized. Doing so causes a merge, which causes 
the `PreCopyMergedSegmentWarmer` to then invoke 
`PrimaryNode.preCopyMergedSegmentFiles`. Now, another thread is calling an 
instance method on an incompletely initialized PrimaryNode.
   
   In our case, this leads to a `NullPointerException` reading a field of our 
PrimaryNode subclass, despite it being `final` and initialized in the 
constructor. Essentially, this code can fail, but only in rare circumstances:
   
   ```
   public class MyNode extends PrimaryNode {
       private final ImportantField importantField;
   
       public MyNode(IndexWriter writer) {
           super(writer, ...); // leaks `this` into `writer`
           importantField = new ImportantField();
       }
   
       protected void preCopyMergedSegmentFiles(...) {
           // this can be called before constructor finishes! danger!
           importantField.getInformation(); // very surprising 
NullPointerException!
       }
   }
   ```
   
   I am not entirely sure what the fix here is. Initialization is clearly a 
tricky problem.
   Generally, it might be nice to move any code beyond setting fields out of 
the PrimaryNode constructor into a start() method, but this is likely not a 
compatible change.
   Leaking a `this` reference from an object constructor into shared state is 
an inherently risky thing to do.
   
   ### Version and environment details
   
   Lucene 9.4.1, Java 17+19, platform-independent


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to