You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: disable native callbacks before disposing managed state - #171
Prevent native callbacks from entering managed code after YetAnotherHttpHandler.Dispose() returns.
Previously, disposing a handler did not necessarily stop its native callbacks: other handlers could keep the
shared Tokio runtime running, and outstanding requests could delay native resource release through SafeHandle
references. This could allow callbacks to access released managed state or enter IL2CPP during runtime cleanup.
Changes
Add a native callback gate covering response headers, response bodies, completion notifications, and custom
TLS certificate verification.
Close callback admission before canceling requests, then wait for admitted callbacks and deferred body-flush
acknowledgments before releasing managed state.
Keep native contexts alive through shared ownership while remaining native work finishes.
Reject synchronous disposal from native callbacks to prevent deadlocks.
Disposal guarantees that the handler cannot invoke further managed callbacks. Native connection cleanup and DNS
work may finish afterward. Applications must still dispose handlers before Unity/IL2CPP runtime cleanup begins.
Managed builds passed for netstandard2.0, netstandard2.1, and net8.0.
Native release builds passed for iOS ARM64 devices and ARM64 simulators.
Reproduction sample
A standalone MagicOnion server and Unity client sample
exercises this shutdown scenario. The server sends a StreamingHub message every 100 ms. After five seconds,
the client calls Application.Quit() and synchronously disposes the connected YAHA handler in Application.quitting,
without an explicit GC.Collect().
Tested with Unity 2022.3.74f1, IL2CPP ARM64, incremental GC enabled, and an iPhone 16 Pro / iOS 18.5 Simulator:
Official YAHA 1.11.4: Dispose returned and handler ownership dropped to zero, but a Tokio worker subsequently
crashed in the OnReceive reverse P/Invoke wrapper's Thread::Attach path while Unity was cleaning up.
Patched build at 603b965: Dispose returned, the hub disconnected, and the process exited voluntarily without
a new crash report.
Each version was tested once with the same client lifecycle. This reproduces a shutdown/thread-attachment crash,
not the exact GC_is_marked failure, and does not establish reliability on physical devices. The patched run still
logged IL2CPP Free after allocator was destroyed messages; their source has not been identified.
Packaging
This change adds native entry points and changes the response-completion handle layout. Managed code and native
binaries must be distributed together. Update all Unity prebuilt native libraries through the existing CI
workflow before publishing.
Because Dispose() now rejects any call while _callbackDepth != 0, this constructor cleanup can fail when a handler is lazily initialized from another handler's native callback and Initialize throws (for example, unsupported UDS). The InvalidOperationException then masks the original error and leaves the acquired context/runtime unreleased; route constructor-failure cleanup through an internal path that bypasses the user-facing callback deadlock guard (there are no requests to join yet).
The reason will be displayed to describe this comment to others. Learn more.
Hey, I've thrown my agent on your PR. I specialise in C#, but I have a review flow which hadn't failed me even once so far, regardless of the language, so I thought it is worth it to bring my two cents here.
One "must fix" has been found - a race condition during disposal. Rest are polishes/readability/caretaking changes. Do with the findings what you see fit.
Thank you for your contribution! We're looking forward to the fix 🙏
The reason will be displayed to describe this comment to others. Learn more.
must fix · The disposal drain trusts a lock that lives in another class.
Dispose copies _requests once, then only releases the requests in that copy. A request that registers at the moment Dispose runs can miss the copy. Its callbacks then hit the closed gate and return. OnComplete never runs. Nothing calls Release() on it. The caller's SendAsync waits on a response that never arrives, and the request's self-handle is never freed.
Through the public API this cannot happen. SendAsync registers under _lifetimeLock, and Dispose sets _disposed under the same lock first. The problem is that this class is internal and the tests drive it directly. The class does not enforce the invariant it depends on.
Give NativeHttpHandlerCore its own _disposed flag under a lock shared with _requests.TryAdd, and have Send throw ObjectDisposedException once disposal starts. The Send catch at line 305 then reduces to reqCtxHandle.Dispose(); throw;, and its comment goes with it.
The reason will be displayed to describe this comment to others. Learn more.
I’m keeping the current design, where YetAnotherHttpHandler coordinates request admission and disposal through _lifetimeLock. Registration completes before disposal can begin, and new sends are rejected once disposal starts.
I’ve added a comment at the disposal snapshot to explicitly document this guarantee. Direct callers of the internal core must preserve the same coordination; the existing direct-core test is sequential.
Moving synchronization into the core could improve encapsulation, but I don’t consider it necessary for correctness in the current call path.
The reason will be displayed to describe this comment to others. Learn more.
medium · The shutdown sequence has no owner, so five files each narrate their slice of it.
Disposal runs one protocol: disable callback admission, cancel and drain in-flight work, then release state. No type owns that sequence. Every participant re-explains its part in a comment, and the comments cross-reference each other across files.
The three comments in this method become the three method names. The FreeCertificateHandle safety note is implied by order. It runs after the drain. Once the sequence is named here, the slice-comments in the other four files have nothing left to state.
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 5b96690. Extracted the three shutdown phases so Dispose() explicitly coordinates disabling and draining callbacks, releasing requests, and freeing the certificate handle. The execution order is unchanged, including taking the request snapshot after disabling callbacks.
Removed redundant sequencing comments while retaining explanations of concurrency constraints and deadlock risks.
The reason will be displayed to describe this comment to others. Learn more.
low · Every callback is written twice to bump a counter.
Each of the four callbacks now has a public wrapper plus a _-prefixed body, only to move _callbackDepth up and down. That is eight methods and four identical try/finally blocks. One scope type collapses them to four:
Each callback then keeps its real body and opens with using var _ = CallbackScope.Enter();. The factory form is valid on C# 9 and netstandard2.0. It stays safe for the Unity and IL2CPP targets. ThrowIfInCallback is untouched.
The reason will be displayed to describe this comment to others. Learn more.
Applied in 83b0d9b: added CallbackScope and merged each callback’s body into its attributed entry point, preserving the depth tracking and exception cleanup behavior.
The reason will be displayed to describe this comment to others. Learn more.
low · The manual Arc-resurrection appears three times; give it a name.
increment_strong_count then from_raw is the idiom for cloning an owning reference that outlives the SafeHandle. It appears here, in yaha_dispose_context, and in the test. Extract unsafe fn clone_context(ctx) -> Arc<YahaNativeContextInternal>. The comment here and the one on yaha_dispose_context both go. The idiom stops being copy-pasted.
The reason will be displayed to describe this comment to others. Learn more.
nit · The gate's three doc comments restate what the names already show.
Rename defer to retain_for_deferred_ack, and leave its race note to the test that proves it (deferred_acknowledgement_outlives_callback_even_when_close_races_it). The type doc on CallbackGate restates enter. The doc on close describes a caller protocol that belongs to the disposal sequence in NativeHttpHandlerCore.Dispose, and wait's assert!(closed) already encodes the order. Drop all three.
The reason will be displayed to describe this comment to others. Learn more.
low · Drop the second lock and the comment explaining it disappears.
The comment answers why handler.Dispose() is outside the lock and why the field is nulled under a second lock. Once _disposed is set under the first lock, SendAsync throws before it reads _handler again. The second lock around _handler = null guards nothing. Remove it. With no re-lock, there is no question to answer.
The reason will be displayed to describe this comment to others. Learn more.
suggestion · This one marks a real ordering constraint, not weak code.
WaitForLatestFlush() blocks while holding _writeLock. A callback thread can be blocked in it while holding the lock. CancelPendingFlush() has to run before Cancel contends for that lock, or the two deadlock. The ordering is load-bearing. Suggested trim:
Suggested change
// A disposing handler can cancel a response whose caller's token
// was never cancelled. Unblock backpressure BEFORE taking the lock:
// a completion callback may hold it while waiting for this flush.
_pipe.Writer.CancelPendingFlush();
// Unblock a full pipe before taking _writeLock: a callback thread can be
// blocked in WaitForLatestFlush() while holding it. Covered by Dispose_UnblocksUnreadResponseBody.
_pipe.Writer.CancelPendingFlush();
The comment vanishes entirely only if the flush stops blocking under _writeLock. That is a separate lock redesign and a fair follow-up rather than a block on this PR.
The reason will be displayed to describe this comment to others. Learn more.
nit · Two comments explain a choice the test already proves.
This shutdown_background note and the two test comments in this file restate what runtime_release_does_not_join_native_blocking_work verifies. The tokio method name already says background. Delete them and lean on the test.
The reason will be displayed to describe this comment to others. Learn more.
nit · The explanatory comments in this file restate the test name or the assert above them.
Each comment narrates what the test name or the assertion already carries. The empty-catch note further down is already spelled by its when (ex is …) filter. Fold each into the test name, a helper name, or an assert message.
Managed sources and native binaries must come from the same build. The callback gate requires `yaha_context_disable_callbacks`, `yaha_context_wait_callbacks` and a matching response-completion handle layout. Before distributing a Unity package, update the prebuilt libraries for every platform through the `Build Native Libraries` workflow with `update-unity-native` enabled.
The reason will be displayed to describe this comment to others. Learn more.
nit · Keep the section, trim the sentences that will rot.
The "Disposing a handler" prose is the right home for public dispose behavior. This paragraph names internal symbols (yaha_context_disable_callbacks, response-completion handle layout) that track the native ABI and will drift. Trim to observable behavior.
requestSequence is a process-wide int, so it can eventually wrap. Since this new dictionary uses it as the only key and ignores TryAdd's result, a request admitted after a collision is not tracked for disposal, and its later RemoveRequest can remove the older request with the same key. Use a non-recycling request identity as the dictionary key, or handle collisions without removing another request.
We would like to review and verify the changes on our side as well, so it may be difficult for us to merge the submitted code immediately.
In particular, cleanup and shutdown logic can be quite delicate. Issues in these areas can easily lead to problems such as the Unity Editor freezing or resources not being released properly, so we hope you understand that we need to proceed carefully.
At this point, we have two pieces of feedback:
It looks like you are currently going through a review loop with Copilot or other AI coding agents. Please keep the pull request as a Draft until the changes are ready for review.
Please remove any prebuilt binaries from the commit. Build artifacts will be generated by the repository's CI.
A custom PipeOptions writerScheduler that dispatches to the disposing
thread could deadlock Dispose: Cancel waited for the latest flush task and
wait_callbacks waited for the deferred acknowledgement, both of which only
complete once that scheduler runs the flush continuation.
Cancel now completes the writer without waiting for the flush task and
claims the pending acknowledgement itself, so disposal no longer depends
on the writer scheduler.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispose drains requests, so the context SafeHandle releases the verifier
GCHandle immediately. The closed callback gate prevents retained native
owners from invoking it, so assert no other verifier runs instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cancel can complete a pending flush and acknowledge its deferred task
before OnReceive checks write.IsCompleted, which then completed the
same task handle again. Branch on whether the ack was deferred instead,
and claim it in the error path as well.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
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
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.
Summary
Prevent native callbacks from entering managed code after YetAnotherHttpHandler.Dispose() returns.
Previously, disposing a handler did not necessarily stop its native callbacks: other handlers could keep the
shared Tokio runtime running, and outstanding requests could delay native resource release through SafeHandle
references. This could allow callbacks to access released managed state or enter IL2CPP during runtime cleanup.
Changes
Add a native callback gate covering response headers, response bodies, completion notifications, and custom
TLS certificate verification.
Close callback admission before canceling requests, then wait for admitted callbacks and deferred body-flush
acknowledgments before releasing managed state.
Keep native contexts alive through shared ownership while remaining native work finishes.
Handle concurrent disposal, blocked uploads, unread response bodies, and request initialization failures.
Reject synchronous disposal from native callbacks to prevent deadlocks.
Disposal guarantees that the handler cannot invoke further managed callbacks. Native connection cleanup and DNS
work may finish afterward. Applications must still dispose handlers before Unity/IL2CPP runtime cleanup begins.
Validation
.NET regression tests excluding stress tests: 130 passed, 3 skipped, including 8 disposal cases.
Rust tests: 6 passed, covering callback admission, deferred acknowledgements, retained contexts, TLS
verification, and background runtime shutdown.
Managed builds passed for netstandard2.0, netstandard2.1, and net8.0.
Native release builds passed for iOS ARM64 devices and ARM64 simulators.
Reproduction sample
A standalone MagicOnion server and Unity client sample
exercises this shutdown scenario. The server sends a StreamingHub message every 100 ms. After five seconds,
the client calls
Application.Quit()and synchronously disposes the connected YAHA handler inApplication.quitting,without an explicit
GC.Collect().Tested with Unity 2022.3.74f1, IL2CPP ARM64, incremental GC enabled, and an iPhone 16 Pro / iOS 18.5 Simulator:
crashed in the OnReceive reverse P/Invoke wrapper's
Thread::Attachpath while Unity was cleaning up.603b965: Dispose returned, the hub disconnected, and the process exited voluntarily withouta new crash report.
Each version was tested once with the same client lifecycle. This reproduces a shutdown/thread-attachment crash,
not the exact
GC_is_markedfailure, and does not establish reliability on physical devices. The patched run stilllogged
IL2CPP Free after allocator was destroyedmessages; their source has not been identified.Packaging
This change adds native entry points and changes the response-completion handle layout. Managed code and native
binaries must be distributed together. Update all Unity prebuilt native libraries through the existing CI
workflow before publishing.