Synchronize accesses to reachability handlers #7736
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
There was a problem hiding this comment.
Instead of the explicit synchronized blocks, it would be better to use proper synchronized data structures. For ReachabilityHandlerFeature.activeHandlers and triggeredHandlers that would be wrapping using Collections.synchronizedMap, while for the nested set that does not require identity semantics we can just use ConcurrentHashMap.createKeySet.
|
Hi @christianwimmer I see there is |
zakkak
force-pushed
the
2023-11-06-fix-5868
branch
2 times, most recently
from
e6190e3 to
c4f2371
Compare
Yes, I forgot that we have that. |
| @@ -119,7 +122,7 @@ public void duringAnalysis(DuringAnalysisAccess a) { | |||
| Set<Object> triggers = activeHandlers.get(callback); | |||
| if (callback instanceof Consumer) { | |||
| if (isTriggered(access, triggers)) { | |||
| triggeredHandlers.put(callback, null); | |||
| triggeredHandlers.put(callback, NULL_MAP); | |||
There was a problem hiding this comment.
Why not just use Map.of()? We want that map to be immutable anyways.
There was a problem hiding this comment.
I didn't think about it. It makes much more sense. Done, thanks.
| private final IdentityHashMap<Object, Set<Object>> activeHandlers = new IdentityHashMap<>(); | ||
| private final IdentityHashMap<Object, Map<Object, Set<Object>>> triggeredHandlers = new IdentityHashMap<>(); | ||
| private static final Map<Object, Set<Object>> NULL_MAP = new IdentityHashMap<>(); | ||
| private final ConcurrentIdentityHashMap<Object, ConcurrentHashMap.KeySetView<Object, Boolean>> activeHandlers = new ConcurrentIdentityHashMap<>(); |
There was a problem hiding this comment.
Why change the generic type from Set to ConcurrentHashMap.KeySetView? The field can just be defined as Map<Object, Set<Object>> activeHandlers.
Same for all the other type definitions.
There was a problem hiding this comment.
I guess I just like explicit types :)
Changed.
| @@ -88,7 +91,7 @@ private void registerReachabilityHandler(BeforeAnalysisAccess a, Object callback | |||
| BeforeAnalysisAccessImpl access = (BeforeAnalysisAccessImpl) a; | |||
| AnalysisMetaAccess metaAccess = access.getMetaAccess(); | |||
|
|
|||
| Set<Object> triggerSet = activeHandlers.computeIfAbsent(callback, c -> new HashSet<>()); | |||
| ConcurrentHashMap.KeySetView<Object, Boolean> triggerSet = activeHandlers.computeIfAbsent(callback, c -> ConcurrentHashMap.newKeySet()); | |||
There was a problem hiding this comment.
that's actually the prototypical use case for var triggerSet, which we can now use in the native-image code base.
Prevent data races in reachability handlers registration when using `-H:-RunReachabilityHandlersConcurrently`. Closes oracle#5868
zakkak
force-pushed
the
2023-11-06-fix-5868
branch
from
c4f2371 to
777cb82
Compare
|
@oubidar-Abderrahim can you please create the internal PR for this |
|
Thank you for the review @christianwimmer |
|
This should also be backported to 23.1 and 23.0 |
|
@oubidar-Abderrahim is there any progress in regards to getting this merged? Thanks |
I'm in the process of running the final tests and benchmarks, should be merged soon. And it will be backported. |
|
Great, thanks for the update @christianwimmer. |
|
We encountered the same issue. Thanks for fixing it. @christianwimmer when do we expect this will be released? |
|
@btomala it should land in GraalVM for JDK 21.0.2 in late January. |
Prevents data races in reachability handlers registration when using
-H:-RunReachabilityHandlersConcurrently.The fact that #5868 is not deterministic and it's related to enabling/disabling concurrency hints that it's probably caused by a data race.
Adding a synchonized blocks in
com.oracle.svm.hosted.ReachabilityHandlerFeature#registerReachabilityHandler(org.graalvm.nativeimage.hosted.Feature.BeforeAnalysisAccess, java.lang.Object, java.lang.Object[])seems to resolve the issue.Closes #5868