Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redo #12707: Do not rely on isAlive() status of MemorySegment#Scope and make sure IndexInput#close() does not throw IllegalStateException and waits instead #12785

Merged
merged 11 commits into from
Nov 9, 2023

Conversation

uschindler
Copy link
Contributor

Unfortunately the solution in #12707 was not working well with concurrency. The is alive status of MemorySegment.Scope may be stale. In that case the IllegalStateException was catched, but the confirmation using isAlive() did not work, so the original IllegalStateException was rethrown instead of the required AlreadyClosedException.

This PR reverts #12707 and falls back to the naive approach by checking exception message for "closed". I am in contact with @mcimadamore to find a better solution to confirm the closed status without relying on Exception message.

@uschindler uschindler added this to the 9.9.0 milestone Nov 8, 2023
@uschindler uschindler self-assigned this Nov 8, 2023
@uschindler
Copy link
Contributor Author

The bug in JDK is here: https://bugs.openjdk.org/browse/JDK-8319756

@uschindler
Copy link
Contributor Author

uschindler commented Nov 9, 2023

After some discussion with @mcimadamore we figured out that there are more problems, so we need to rely on the exception message.

The following problem can occur and possibly happens here: When you close the main IndexInput from another thread this may fail with an IllegalStateException. While trying to do this also the clone may get the above ISE and should convert it to the AlreadyClosedException. But when the main thread got the ISE the close operation did not work and the previous state is restored. And that's waht we see here. The main thread in the test tries to close but gets an exception so the close did not succeed.

The main problem that the clsoed state is a 3state setting: OPEN, CLOSING, CLOSED. When the scope is in CLOSING state it may go back to OPEN. So after the IllegalStateException the status may go back to "OPEN" and this is why the Exception is not converted.

I will add another improvement to the test to "retry" closing on ISE. We should maybe also add a loop in the MemorySegmentIndexInput that it tries to close multiple times while yielding the threads.

@uschindler
Copy link
Contributor Author

I committed another change to make the sequence of IndexInput#close() first try to close the segment and then set everything to null. In case if ISE, the IndexInput is not closed.

I will next check how to re-try closing (which may take very long time under high load on the other threads).

…ates our contract. Instead retry the close operation. Improve TestMmapDirectory.testAceWithThreads to not write new file on every round (speedup >5 times).
@uschindler
Copy link
Contributor Author

I fixed the close() method to no longer throw IllegalStateException as this would violate the contract. When we close only IOException is allowed. As half-open index inputs are worse, we just retry to close the scope/session in a spinloop.

I checked this with Maurizio, until the bugs in JVM are fixed, this is the best thing to do.

Maurizio openend another issue in the JDK, describing our problem: https://bugs.openjdk.org/browse/JDK-8319782 (duplicate of https://bugs.openjdk.org/browse/JDK-8310644).

@uschindler uschindler changed the title Redo #12707: Do not rely on isAlive() status of MemorySegment#Scope Redo #12707: Do not rely on isAlive() status of MemorySegment#Scope and make sure IndexInput#close() does not throw IllegalStateException and waits instead Nov 9, 2023
@uschindler
Copy link
Contributor Author

I let TestMmapDirectory.testAceWithThreads run with gradlew :lucene:core:beast with many iterations and high multiplier: JDK 19, 20, 21 showed no problems.

@uschindler uschindler merged commit 5b43384 into apache:main Nov 9, 2023
4 checks passed
asfgit pushed a commit that referenced this pull request Nov 9, 2023
…nd make sure IndexInput#close() does not throw IllegalStateException and waits instead (#12785)
@uschindler uschindler deleted the dev/memorysegment-scope-closed branch November 9, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant