lucene-replicator PrimaryNode unsafely publishes reference during construction
- Dominant language
- Java
- Stars
- 3.6k
- Forks
- 1.4k
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 88
Description
### 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
Contributor guide
Research direction
Start in the lucene-replicator PrimaryNode constructor, especially line 92 where IndexWriter is given a PreCopyMergedSegmentWarmer referencing the node. Trace IndexWriter.commit(), the merge warmer, and PrimaryNode.preCopyMergedSegmentFiles to understand the construction-time callback. Done should prevent callbacks from observing an incompletely initialized PrimaryNode and cover the rare failure with a concurrency regression test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- search
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100