You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/11/09 21:09:57 UTC
[GitHub] [lucene] stevenschlansker opened a new issue, #11913: lucene-replicator PrimaryNode unsafely publishes reference during construction
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-unsubscribe@lucene.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] stevenschlansker commented on issue #11913: lucene-replicator PrimaryNode unsafely publishes reference during construction
Posted by GitBox <gi...@apache.org>.
stevenschlansker commented on issue #11913:
URL: https://github.com/apache/lucene/issues/11913#issuecomment-1338221987
I think that would fix this specific problem, although where would you set `initialized`? The end of the PrimaryNode constructor is too soon, since the subclass constructor is not finished.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] stevenschlansker commented on issue #11913: lucene-replicator PrimaryNode unsafely publishes reference during construction
Posted by GitBox <gi...@apache.org>.
stevenschlansker commented on issue #11913:
URL: https://github.com/apache/lucene/issues/11913#issuecomment-1347017604
Letting PrimaryNode initialize the writer would work for us. It's in fact pretty similar to the workaround we implemented to fix this in our application.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] romseygeek commented on issue #11913: lucene-replicator PrimaryNode unsafely publishes reference during construction
Posted by GitBox <gi...@apache.org>.
romseygeek commented on issue #11913:
URL: https://github.com/apache/lucene/issues/11913#issuecomment-1346186792
This isn't an area of the code I know well, but I think part of the problem here is that the IndexWriter lifecycle is entirely under the management of the PrimaryNode (e.g. calling `close` on the PrimaryNode also closes the IndexWriter), but PrimaryNode takes the writer as a constructor parameter rather than building it itself, which means it's available to outside classes before construction is finished. So maybe we should instead change the PrimaryNode constructor to take a Directory and IndexWriterConfig, and build the IndexWriter inside the constructor method? And then the IndexWriter can be exposed on a method on the PrimaryNode but it won't be available to outside consumers until PrimaryNode has finished building.
cc @mikemccand who actually understands this thing :)
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] zhaih commented on issue #11913: lucene-replicator PrimaryNode unsafely publishes reference during construction
Posted by GitBox <gi...@apache.org>.
zhaih commented on issue #11913:
URL: https://github.com/apache/lucene/issues/11913#issuecomment-1337918394
Can we have a `initialized` field inside `PrimaryNode` and let `PreCopyMergedSegmentWarmer` check against it before making any call to `PrimaryNode`?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org