apache / apache/lucene

lucene-replicator PrimaryNode unsafely publishes reference during construction

Open
#11,913 4 comments 0 reactions 0 assignees View on GitHub
type:bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.