-
Notifications
You must be signed in to change notification settings - Fork 185
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
Java exceptions from CompactHashStore #3447
Comments
graalvmbot
pushed a commit
that referenced
this issue
Feb 13, 2024
* CompactHashStore seems to be more prone to races: #3447 * Until a thread-safe Hash implementation is merged.
I changed the default to BucketsHashStore in #3456 until thread-safe Hash lands. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We know that the Hash implementations are not thread-safe,
but in a short period we saw several related errors coming from CompactHashStore.
It seems to error more often than PackedHashStore and BucketsHashStore.
It might be worth switching the default back to BucketsHashStore for now to reduce java exceptions.
These are the exceptions I've seen:
I reduced about a dozen exceptions I collected down to 4 distinct ones and put the full traces in this gist.
The files are a bit long which makes it hard to look at, so here are links to the individual files:
Looking at the code quickly it seems like there is probably a race condition between deleting
truffleruby/src/main/java/org/truffleruby/core/hash/library/CompactHashStore.java
Lines 412 to 415 in b35d756
and inserting
truffleruby/src/main/java/org/truffleruby/core/hash/library/CompactHashStore.java
Lines 608 to 612 in b35d756
When the error occurs at deletion it would be easy to add checks that the integer is within bounds and return null if not,
but for insertion the decision of how to handle an exception is less clear.
It might be worth trying to identify what CRuby does.
What keys remain in the hash may be undefined when using a Hash across multiple threads, but in CRuby it doesn't crash (or at least not nearly as often as this seems to).
The text was updated successfully, but these errors were encountered: