ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358372290
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -212,59 +211,40 @@ public void checkIntegrity(MergePolicy.OneMerge merge)
throws IOException {
@Override
public FlatVectorsReader getMergeInstance() throws IOException {
- if (mergeNeedsItsOwnMapping() == false) {
- return this;
- }
- IndexInput data = original.mergeVectorData();
- boolean success = false;
- try {
- FlatVectorsReader mergeInstance = new Lucene99FlatVectorsReader(this,
data.clone());
- success = true;
- return mergeInstance;
- } finally {
- if (success == false) {
- IOUtils.closeWhileHandlingException(original.release());
- }
- }
+ IndexInput data = original.sequentialVectorData();
+ return data == vectorData ? this : new Lucene99FlatVectorsReader(this,
data.clone());
Review Comment:
`getMergeInstance()` and `getFloatVectorValues(field, SEQUENTIAL)` get the
cached sequential input from a synchronized method, then call `.clone()` or
`.slice()` on it after the lock is released. A concurrent `close()` can close
that input in between and make the clone or slice fail with
AlreadyClosedException, so we probable need to clone while holding the lock?
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -389,57 +382,19 @@ public RandomVectorScorer getRandomVectorScorer(String
field, short[] target) th
target);
}
- /**
- * Closes the mapping a merge used, once no merge instance holds it. A later
merge maps the file
- * again. Only a merge instance holds the mapping, and it gives it back
once: finishing the reader
- * it came from, or finishing it again, releases nothing.
- */
- @Override
- public void finishMerge() throws IOException {
- if (original != this) {
- IOUtils.close(original.releaseMergeVectorData(this));
- }
- }
-
- /** Gives back the hold of {@code mergeInstance}, once; returns the mapping
to close, if any. */
- private synchronized IndexInput
releaseMergeVectorData(Lucene99FlatVectorsReader mergeInstance) {
- assert original == this && mergeInstance.original == this;
- if (mergeInstance.finished) {
- return null;
- }
- mergeInstance.finished = true;
- return release();
- }
-
- /**
- * Gives back one hold on the mapping. Once none is left, returns it for the
caller to close
- * outside the lock.
- */
- private synchronized IndexInput release() {
- assert original == this && mergeInstances > 0;
- if (--mergeInstances > 0) {
- return null;
- }
- IndexInput toClose = mergeVectorData == vectorData ? null :
mergeVectorData;
- mergeVectorData = null;
- return toClose;
- }
-
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, takeMergeVectorData());
+ IOUtils.close(vectorData, takeSequentialVectorData());
Review Comment:
similar in `Lucene90CompressingStoredFieldsReader`
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -389,57 +382,19 @@ public RandomVectorScorer getRandomVectorScorer(String
field, short[] target) th
target);
}
- /**
- * Closes the mapping a merge used, once no merge instance holds it. A later
merge maps the file
- * again. Only a merge instance holds the mapping, and it gives it back
once: finishing the reader
- * it came from, or finishing it again, releases nothing.
- */
- @Override
- public void finishMerge() throws IOException {
- if (original != this) {
- IOUtils.close(original.releaseMergeVectorData(this));
- }
- }
-
- /** Gives back the hold of {@code mergeInstance}, once; returns the mapping
to close, if any. */
- private synchronized IndexInput
releaseMergeVectorData(Lucene99FlatVectorsReader mergeInstance) {
- assert original == this && mergeInstance.original == this;
- if (mergeInstance.finished) {
- return null;
- }
- mergeInstance.finished = true;
- return release();
- }
-
- /**
- * Gives back one hold on the mapping. Once none is left, returns it for the
caller to close
- * outside the lock.
- */
- private synchronized IndexInput release() {
- assert original == this && mergeInstances > 0;
- if (--mergeInstances > 0) {
- return null;
- }
- IndexInput toClose = mergeVectorData == vectorData ? null :
mergeVectorData;
- mergeVectorData = null;
- return toClose;
- }
-
@Override
public void close() throws IOException {
- IOUtils.close(vectorData, takeMergeVectorData());
+ IOUtils.close(vectorData, takeSequentialVectorData());
Review Comment:
I find the close a little hard to follow, and am worried about the multiple
close scenarios, etc. Maybe we can simplify this if we move the implementation
into close itself? e.g.
```
public void close() throws IOException {
final IndexInput sequential;
synchronized (this) {
if (closed) {
return;
}
closed = true;
sequential =
original == this && sequentialVectorData != vectorData ?
sequentialVectorData : null;
sequentialVectorData = null;
}
IOUtils.close(vectorData, sequential);
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
