[Mono.Android] Abort in-flight body I/O on response dispose

[simonrozsival]

Description

AndroidMessageHandler exposes the response body as a BufferedStream over
Android's Java InputStream (backed by okhttp). HttpResponseMessage.Dispose()
can be called from any thread — and, importantly, is how gRPC server-streaming
cancellation tears a call down: it disposes the response rather than cancelling
the individual read. Dispose() disposes the content, which calls Close() on
the body stream, which calls Close() on the Java InputStream.

okhttp implements InputStream.close() on a chunked/fixed-length body by
draining it — issuing another read() so the connection can be reused. Its
AsyncTimeout keeps a process-global list of timeout nodes that each read
enters/exits. When the drain-read runs while the original read is still parked
in AsyncTimeout.enter(), the node is entered twice and okhttp throws
java.lang.IllegalStateException: Unbalanced enter/exit, crashing the
process. (The same concurrent close could also corrupt managed
BufferedStream state and surface as an ArgumentOutOfRangeException.)

The invariant that avoids it: never call Close() on the underlying stream
while a body operation is in flight. A managed CancellationToken cannot by
itself interrupt the blocking Java read — only HttpURLConnection.Disconnect()
(closing the socket) makes a parked read throw and unwind.

Fix

Reworked CancellationAwareResponseStream into a "drain, then close" scheme
that mirrors the idiomatic Java/OkHttp pattern (the reader owns close(); a
separate thread only signals cancellation via disconnect()):

• Every read/write/copy/flush brackets itself with BeginUse()/EndUse().
• Dispose() records the request and cancels an internal
  CancellationTokenSource, which aborts the parked operation via a
  backgrounded HttpURLConnection.Disconnect(). Dispose() never blocks.
• The last in-flight operation closes the underlying stream as it unwinds
  (EndUse → DisposeCore) — the only point at which closing the Java stream
  cannot collide with an active read. When nothing is in flight, Dispose()
  closes directly. This guarantees the stream is always closed exactly once
  (no leak) and never during an active operation.
• Operations started after disposal throw ObjectDisposedException.
• The internal token is linked with the caller's token, so an operation also
  aborts (mapping to OperationCanceledException) when the caller cancels
  directly.
• Writes and Flush are drain-aware too, not just reads — any blocking socket
  operation races dispose identically.
• The HttpURLConnection Java peer is Disconnect()-ed, never disposed:
  deleting its JNI global reference could race a still-unwinding read. The peer
  is reclaimed on finalization. AndroidHttpResponseMessage.Dispose() disconnects
  synchronously as the backstop for non-streaming (buffered) responses; closing a
  socket does not trigger NetworkOnMainThreadException, so it is safe on the
  disposing (possibly UI) thread.

The reasoning is documented inline, since the ordering is non-obvious.

Fixes #12106

Testing

New on-device tests in AndroidMessageHandlerCancellationTests exercise the
dispose/cancel sequences against a stalled local server.

Baseline confirmed against main: built the tests against unmodified main
— ResponseHeadersReadDisposeResponseDuringBodyRead...,
ResponseHeadersReadDisposeResponseDuringCanceledBodyRead..., and
ResponseHeadersReadTokenPassedToReadCancelAndDisposeConcurrently... all crash
with the reported Unbalanced enter/exit, proving these are genuine regression
tests. Notably, ResponseHeadersReadTokenPassedToReadCancelOnly (no dispose)
passes on main: flowing the token to the read is neither the cause nor a
sufficient fix — the crash is Dispose() racing an in-flight read.

With the fix, the full AndroidMessageHandlerCancellation category is 7/7
green on an Android 16 / API 36 physical device, covering:

• prompt cancellation of a body read (ResponseContentRead and
  ResponseHeadersRead completion options) and of a request-body upload;
• disposing the response during an active body read, and during a
  cancellation-aborted body read, without racing the stream close;
• the gRPC shape — the read's token and HttpResponseMessage.Dispose() firing
  concurrently — completing without a native crash.

• Useful description of why the change is necessary
• Links to the issue fixed
• Unit tests

Note

This pull request description was prepared with GitHub Copilot.

[Copilot]

Pull request overview

This PR fixes a race in AndroidMessageHandler where disposing an HttpResponseMessage while a response-body read is in progress could concurrently close the managed BufferedStream and the underlying Java InputStream, leading to OkHttp timeout-state corruption (Java.Lang.IllegalStateException: Unbalanced enter/exit) and potentially corrupted managed buffering state.

Changes:

• Serialize response-stream disposal by tracking active reads in CancellationAwareResponseStream, deferring the underlying stream close until the final read unwinds.
• On disposal during an active read, abort I/O via HttpURLConnection.Disconnect() and reject new reads once disposal is requested.
• Add on-device tests that cover “cancel-then-dispose” (gRPC-like) and “dispose-during-read” sequences.

Show a summary per file

File	Description
tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerCancellationTests.cs	Adds on-device regression tests for disposal during an in-flight response-body read (with and without cancellation).
src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs	Updates CancellationAwareResponseStream to coordinate read/dispose, abort active I/O via Disconnect(), and defer stream closure until reads complete.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 0

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

⚠️ Review — thoughtful fix, a few polish items

Thanks for the very well-documented change. I independently traced the CancellationAwareResponseStream rework and the "drain, then close" scheme is sound: the BeginUse/EndUse bracket guarantees stream.Dispose() (the okhttp drain-read) only runs when inUseCount == 0, which is exactly the invariant needed to avoid the Unbalanced enter/exit crash. Dispose() correctly stays non-blocking by handing the close to the last in-flight operation and backgrounding Disconnect() off the (possibly UI) thread. The exactly-once disposed guard and the deliberate non-disposal of the JNI peer are both well-reasoned. The synchronous wrappers running the operation to completion inside the bracket (so GetAwaiter().GetResult() doesn't block) also checks out.

Findings (all inline):

• ⚠️ 1 performance — per-read linked-CTS + .AsTask() allocations added to the hot ReadAsync(Memory<byte>) path.
• 💡 2 suggestions — a garbled XML <summary> on GetAbortToken, and cleaning up the "EXPERIMENT 1/2" scratch comments in the new tests.

Positives: genuinely reproducing regression tests (confirmed to crash on main), broad coverage of the dispose/cancel/concurrent shapes, drain-awareness extended to writes/flush, and excellent inline rationale for non-obvious ordering.

CI has not reported results yet (status pending, no checks). This isn't mergeable until the Azure DevOps pipeline is green — please confirm the on-device AndroidMessageHandlerCancellation category passes in CI, not just locally.

Verdict: ⚠️ Needs minor changes.

Generated by Android PR Reviewer for #12105 · 111.3 AIC · ⌖ 13.4 AIC · ⊞ 6.8K
_{Comment /review to run again}

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Code Review — ✅ LGTM (with minor comments)

Verdict: ✅ LGTM · Issues: 0 ❌ · 1 ⚠️ · 1 💡

I reviewed the reworked CancellationAwareResponseStream "drain, then close" scheme against the full source and traced the concurrency paths. The design is sound and the state machine is carefully guarded:

• The stateLock-protected inUseCount/disposeRequested/disposed machine correctly guarantees the underlying Java stream is closed exactly once and never while an operation is in flight — DisposeCore runs either from Dispose (nothing in flight) or from the last EndUse (parked op unwinds), so no leak and no Close()-during-read race.
• BeginUse throwing ObjectDisposedException under the lock closes the start-vs-dispose window cleanly.
• RequestDisconnect coalescing via Interlocked.Exchange and mapping transport exceptions to OperationCanceledException with the correct causing token (GetCanceledToken) both look right.
• The three race scenarios (dispose-during-read, cancel-during-read, concurrent cancel+dispose) are covered by genuine regression tests that the PR confirms crash on main — good evidence the fix addresses root cause, not symptoms.

Positive callouts: excellent inline documentation of a genuinely subtle native/JNI race; the decision to Disconnect() rather than Dispose() the peer (avoiding a JNI global-ref race with an unwinding native read) is well justified.

Comments (non-blocking):

1. ⚠️ Contradictory rationale between the synchronous Disconnect() in AndroidHttpResponseMessage.Dispose and the backgrounded one in RequestDisconnect regarding UI-thread socket I/O, plus a slightly inaccurate "already disconnected" comment for the in-flight path.
2. 💡 Minor duplication in the sync void RunOperation bracket.

CI note: no required checks have reported yet on the head commit (status pending) — not a blocker for review, but confirm the on-device AndroidMessageHandlerCancellation suite is green before merge.

Generated by Android PR Reviewer for #12105 · 145.1 AIC · ⌖ 13.3 AIC · ⊞ 6.8K
_{Comment /review to run again}

[github-actions]

🤖 ⚠️ Documentation — Two rationales in this PR conflict about running HttpURLConnection.Disconnect() on the (possibly UI) disposing thread. Here the comment says the synchronous Disconnect() is safe on the UI thread because "closing a socket ... does not trigger NetworkOnMainThreadException", yet CancellationAwareResponseStream.RequestDisconnect goes out of its way to background the very same call because "Disconnect() performs socket I/O that must not run there." Both can't be true — please reconcile.

Relatedly, the comment on the lines above ("for a streaming response the content already disconnected via base.Dispose above") is inaccurate for the in-flight case: when a body read is parked, base.Dispose only schedules a backgrounded disconnect and defers the close, so this synchronous Disconnect() is frequently the call that actually aborts the parked read — on the disposing thread. Worth clarifying so the "Dispose() never blocks" invariant in the class docs isn't misleading.

(Verify: same httpConnection peer is shared by AndroidHttpResponseMessage and CancellationAwareResponseStream.)

[github-actions]

🤖 💡 Code organization — This sync void RunOperation duplicates the BeginUse/try/catch-map/finally/EndUse bracket verbatim from the value-returning RunOperation<T>(Func<T>, ...) above. The async pair already delegates to keep the bracket "in exactly one place"; consider the same here to match that stated design intent:

void RunOperation (Action operation, string canceledMessage) =>
    RunOperation<bool> (() => { operation (); return true; }, canceledMessage);

(Rule: Reduce duplication — consolidate near-identical helpers.)