Uh oh!
There was an error while loading. Please reload this page.
Fix concurrent creation of duplicate Java peers - #12459
Closed
simonrozsival wants to merge 8 commits into
Closed
Conversation
Canonicalize concurrently created Java peers against the registered winner and make the CoreCLR peer registry safe for concurrent access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c1b612e-232a-4e9f-983b-45dd00dd9b4e
Keep the existing registry lock and reconcile concurrent peer creation in one explicit pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Double-check the peer registry while holding a value-manager lock so only one concurrent caller creates and registers a peer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Construct implicitly created peers without registration, mark them replaceable, and then register them explicitly instead of carrying creation state in thread-local storage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the value-manager-wide peer creation lock with post-creation canonicalization: GetPeer() creates as before, then re-reads the registry and returns whichever peer won registration, disposing the loser. The lock was held across CreatePeer(), which runs activation constructors and Java class loading, so it could invert against a Java monitor held by another thread that was itself waiting to create a peer. It also serialized every peer creation process-wide, in shared code used by all runtime configurations, while still leaving GetValue() and JavaAs() free to create peers without it. Registration was already the arbiter of which peer is canonical; the only missing piece was that GetPeer() returned the peer it created rather than the registered one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Generated JavaPeerProxy.CreateInstance() overrides now allocate the peer with RuntimeHelpers.GetUninitializedObject(), mark it Replaceable, and then invoke the activation constructor, instead of using newobj. TrimmableTypeMap's reflection fallback for closed generic targets does the same. The activation constructor is what registers the peer, so a peer that only becomes Replaceable afterwards can be evicted by the next implicitly created peer -- letting two threads that wrap the same Java instance each keep a different wrapper. This replaces the previous attempt, which passed JniHandleOwnership.DoNotRegister so registration could happen explicitly after construction. That flag never reached ConstructPeerCore() for two families of peers: Java.Lang.Throwable's SetHandle() hardcodes JniObjectReferenceOptions.Copy, and so do both generated Java.Interop-style activation paths. It also left peers unregistered while their constructors ran. Pre-marking mirrors TypeManager.CreateProxy() in the non-trimmable typemap implementation, which already establishes this invariant the same way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 tasks
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 freeto 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.
Fixes a startup race where two threads create two managed wrappers for the same Java instance and each keeps a different one.
Application.Contextcan permanently cache a non-canonical peer, which is whyAndroid.RuntimeTests.JnienvArrayMarshaling.GetObjectArrayflakes in the CoreCLR + trimmable typemap configuration:Assert.AreSame (context, values [0])compares the cached wrapper against the registered one.The race
Android runs instrumentation
onCreate()before applicationonCreate(), so during startup:App.onCreate()→Application.n_OnCreate()→GetObject<Application>(self)OnStart()→ readsApplication.ContextBoth miss
PeekPeer()for the same JavaApplication, both create a peer, and both register. Two things then go wrong:GetPeer()returned the peer it created rather than the one that won registration.JniManagedPeerStates.Replaceableonly after the activation constructor had already registered them, soAddPeer()could not useReplaceableto keep the first one — the second peer, not yet marked, evicted the first.Changes
Return the registered peer.
JniRuntime.JniValueManager.GetPeer()creates as before, then re-reads the registry and returns whichever peer won registration, disposing the loser. Registration was already the arbiter of peer identity; the only missing piece was handing that winner back to every caller.Creation is deliberately not serialized.
CreatePeer()runs activation constructors and Java class loading, so a lock held across it can invert against a Java monitor owned by another thread that is itself waiting to create a peer. It would also serialize every peer creation process-wide, in shared code used by all runtime configurations, whileGetValue()andJavaAs()reachCreatePeer()without it.Mark implicitly created peers before activation. Generated
JavaPeerProxy.CreateInstance()overrides now allocate the peer withRuntimeHelpers.GetUninitializedObject(), mark itReplaceable, and then invoke the activation constructor, instead of usingnewobj.TrimmableTypeMap's reflection fallback for closed generic targets does the same.This mirrors
TypeManager.CreateProxy()in the non-trimmable typemap implementation, which already establishes the invariant this way.Both halves are required: pre-marking makes registration a stable "first implicit peer wins" arbiter, and canonicalization makes every caller receive that winner.
Suppress a now-expected warning. With pre-marking, two concurrently created implicit peers are both
Replaceablewhen they meet inAddPeer(). That is the normal outcome, so it no longer logsWarning: Not registering PeerReference=....Rejected alternative
An earlier revision passed
JniHandleOwnership.DoNotRegisterthroughCreateInstance()so registration could happen explicitly, after construction. That flag never reachesConstructPeerCore()for two families of peers —Java.Lang.Throwable.SetHandle()hardcodesJniObjectReferenceOptions.Copy, and so do both generated Java.Interop-style activation paths — so the invariant silently did not hold for them. It also left peers unregistered while their constructors ran, whereJavaPeerProxy.ShouldSkipActivation()andGetActivationPeer()would no longer find them.Validation
Java.Interop-Tests: 680 passed, 6 skipped.GetPeer_ReturnsRegisteredPeerConcurrentlyuses a barrier to force both callers past the initialPeekPeer()miss, then asserts both created a peer and both received the registered winner.Microsoft.Android.Sdk.TrimmableTypeMap.Tests: 801 passed, including two new tests asserting the generatedCreateInstanceallocates uninitialized, callsMarkCreatedPeerReplaceable(), andcalls the activation constructor rather thannewobj-ing it — for both XA-style and Java.Interop-style leaf constructors.Mono.Android.csprojbuilds clean.Still to do: an on-device run of
JnienvArrayMarshaling.GetObjectArrayin Release/CoreCLRTrimmable.Closes#10973