[NSUrlSessionHandler] Surface cancelled requests with a better exception. Fixes #25667.#25699
[NSUrlSessionHandler] Surface cancelled requests with a better exception. Fixes #25667.#25699rolfbjarne wants to merge 10 commits into
Conversation
…ion. Fixes #25667. Add a test: that sets up a stalling HTTP server, reads the first byte successfully, then cancels the second ReadAsync via a caller-owned CancellationToken. TODO: * Implement the fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Localhost server access should be reliable, so there's no need to paper over failures with IgnoreInCI/Inconclusive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
With Content-Length set, HttpListener may buffer the output until all bytes are written. Switching to chunked transfer encoding ensures the first byte reaches the client immediately, so the first ReadAsync completes and the test can proceed to the cancellation scenario. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…stream-readasync-cancellation-test
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
When the caller's CancellationToken triggers cancellation in the ReadAsync polling loop, rethrow as OperationCanceledException (via ThrowIfCancellationRequested) instead of wrapping it in TimeoutException. TimeoutException is now only thrown when the cancellation comes from an internal source (e.g. HttpClient.Timeout), not from the caller's token. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use 'new OperationCanceledException(message, ex, cancellationToken)' instead of ThrowIfCancellationRequested so the original TaskCanceledException stack trace is preserved as InnerException. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR updates NSUrlSessionHandler’s response stream read behavior so that caller-requested cancellation surfaces as an OperationCanceledException/TaskCanceledException rather than being converted into a TimeoutException (fixing #25667).
Changes:
- Adjusts
NSUrlSessionDataTaskStream.ReadAsynccancellation/timeout exception mapping. - Adds a regression test that reproduces stalled-response reads and asserts caller cancellation surfaces as
OperationCanceledException(or subclass).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Foundation/NSUrlSessionHandler.cs |
Changes exception behavior when ReadAsync is canceled while polling for incoming data. |
tests/monotouch-test/System.Net.Http/NSUrlSessionHandlerTest.cs |
Adds a TCP-based repro test ensuring caller cancellation during a stalled read yields OperationCanceledException. |
| } catch (TaskCanceledException ex) { | ||
| // add a nicer exception for the user to catch, add the cancelation exception | ||
| // to have a decent stack | ||
| // If the caller's token triggered the cancellation, surface it | ||
| // as OperationCanceledException so callers can distinguish | ||
| // between a caller-requested cancellation and a request timeout. | ||
| if (cancellationToken.IsCancellationRequested) |
| var response = await httpClient.SendAsync (request, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait (false); | ||
| var stream = await response.Content.ReadAsStreamAsync ().ConfigureAwait (false); |
| // Stall: never send the remaining body | ||
| await Task.Delay (TimeSpan.FromMinutes (5)).ConfigureAwait (false); |
🔥 [PR Build #4ef4e4a] Build failed (Detect API changes) 🔥Build failed for the job 'Detect API changes' (with job status 'Failed') Pipeline on Agent |
|
🔥 Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #4ef4e4a] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #4ef4e4a] Build passed (Build macOS tests) ✅Pipeline on Agent |
🔥 [CI Build #4ef4e4a] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 22 tests failed, 198 tests passed. Failures❌ monotouch tests (tvOS)21 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ Tests on macOS Tahoe (26) tests1 tests failed, 4 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Surface cancelled requests with an
OperationCanceledExceptioninstead of aTimeoutException. This makes it easier for consumers to determine what happened.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
🤖 Pull request created by Copilot
Fixes: #25667