Fix java context handle #1389
Merged
Fix java context handle #1389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matklad
approved these changes
Dec 21, 2023
Comment on lines
+66
to
+83
| if (contextHandle.getAcquire() != 0L) { | ||
| synchronized (this) { | ||
| if (contextHandle != 0L) { | ||
| // Deinit and signalize that this client is closed by setting the handles to 0. | ||
| clientDeinit(contextHandle); | ||
| this.contextHandle = 0L; | ||
| final var handle = contextHandle.getAcquire(); | ||
| if (handle != 0L) { | ||
| // Signalize that this client is closed by setting the handler to 0, | ||
| // and spin wait until all references that might be using the old handle could | ||
| // be released. | ||
| contextHandle.setRelease(0L); | ||
| while (contextHandleReferences.getAcquire() > 0L) { | ||
| // Thread::onSpinWait method to give JVM a hint that the following code is | ||
| // in a spin loop. This has no side-effect and only provides a hint to | ||
| // optimize spin loops in a processor specific manner. | ||
| Thread.onSpinWait(); | ||
| } | ||
|
|
||
| // This function waits until all submited requests are completed, and no more | ||
| // packets can be acquired after that. | ||
| clientDeinit(handle); |
There was a problem hiding this comment.
Heh, overall, I can't say I am too happy about this:
- abstractly, nesting synchronized and a spinloop (well, and spinloop by itself https://matklad.github.io/2020/01/02/spinlocks-considered-harmful.html :D) doesn't feel right. Ideally, I'd love to see something more principled here
- concretely, I think a flaw in this implementation is that, with many concurrent calls to close, only one would actually wait for the stuff to get close. That is, it can be the case that a
.close()call returns, but there's still some outstanding work.
However, if I got this on the sync correctly, we are going to nix this code anyway soon, so I guess that's fine! Certainly better than an outright race we have at the moment!
There was a problem hiding this comment.
Yeah, my first thought was to encapsulate the context on a thread-safe class controlling all aspects more elegantly, but as you said, we are going to nuke this entire logic very soon.
About the close call, I don't think it can have side effects; the semantics are the same: only the first call to close will wait, no matter if concurrent or sequential calls. If we want to solve that, we can just remove the first verification before the lock.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem
The
contextHandlecan be in use bysubmitwhile thedeinitfunction is called.We synchronize
acquire_packetto prevent new packets from being acquired during shutdown, but we didn't guard thecontextHandleitself, causing the JVM to crash whenacquire_packetwas called with a disposed context.Solution
Guard the contextHandle and a reference counter behind atomics. It's cheaper than a lock.
Undo the temp fix introduced by #1375