Fix leaking trait futures nondeterministically#35
Merged
Conversation
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 6916196 |
Maybe the chain of failure in Java -> Rust -> Java again is just really slow? Delaying after the cancel for a _long_ time may make sure the error has time to clean up.
Seemed to pass more often, but still could fail, with a 5s delay.
to check again and make sure the cause is somehow exceptions
Also print so we know if the java code is called when Rust says the free was called
also add an assertion that we're starting with the handles we expect
going back to all tests again then
BobWall23
approved these changes
Apr 9, 2025
just before calling the foreign free function
being cleaned up
skeet70
changed the title
BobWall23
approved these changes
Apr 11, 2025
coltfred
approved these changes
Apr 11, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 class we were using to cleanup foreign futures on the Java side was parameterized on the future it may need to cancel when freeing. When instantiated though instances of that class weren't being pinned anywhere, so Java was nondeterministically (and almost never locally) choosing to garbage collect them. In that case Rust would call to the memory address it had for
freeon the Java side, with the handle it wanted freed. There would be nothing listening on the Java side though and it'd silently fail.This refactors the object pinned into the handle map to contain both the Java completable future (childFuture) and the async job that handles a response from it (calling back to Rust with the result). Then the free impl class is able to be a static singleton with no context, and is always there when Rust calls for it.
I had to extend the
immediatelytimer again. I'm not sure if that's because of the additional singleton that must be loaded (unlikely) or that github was randomly slower for a bit (likely). Either way it reminded me we should really look at #25 again to improve performance. JNR may be an easy enough conversion for us to use it until JDK 25 releases in September making the new FFM (Foreign Functions and Memory) API available in an LTS.Fixes #19