Skip to content

fix: Propagate cancellation exceptions during response buffering - #2284

Merged
glennawatson merged 1 commit into
reactiveui:mainfrom
radixy0:rethrow-cancellation-exception-while-buffering
Aug 1, 2026
Merged

fix: Propagate cancellation exceptions during response buffering#2284
glennawatson merged 1 commit into
reactiveui:mainfrom
radixy0:rethrow-cancellation-exception-while-buffering

Conversation

@radixy0

@radixy0 radixy0 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix

What is the new behavior?

Caller cancellation during response buffering is propagated. Non-cancellation buffering failures remain best-effort.

What is the current behavior?

If a request is cancelled while deserializing the response, the cancellation exception is swallowed, allowing deserialization of a partially consumed stream.

What might this PR break?

None

Checklist

  • I have read the Contribute guide

  • Tests have been added or updated (for bug fixes / features)

  • Docs have been added or updated (for bug fixes / features)

  • Changes target the main branch

  • PR title follows Conventional Commits

Additional information

Added a regression coverage test

@glennawatson glennawatson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - the bug is real and correctly located, and the product change is nicely minimal.

A few things to sort first:

  • CI is red because of this change. SendAsyncRethrowsCancellationRequestedDuringDeserialization fails on net9.0 and net11.0. Reproduced locally.
  • The new test passes without the product fix, so it does not guard the regression. Verified by reverting RequestExecutionHelpers.cs to main and re-running.
  • The fix is a no-op on net8.0 and below, because the LoadIntoBufferAsync polyfill discards the token.
  • The SuppressMessage should be two doc comments instead.
  • The docs entry belongs in a new V15 section, not V14.

Suggestions inline for each.

Comment thread src/Refit/RequestExecutionHelpers.cs Outdated
Comment thread docs/breaking-changes.md Outdated
Comment thread src/tests/Refit.Tests/ResponseTests.cs Outdated
Comment thread src/tests/Refit.Tests/ThrowOnGetLengthMemoryStream.cs Outdated
Comment thread src/tests/Refit.Tests/ThrowOnGetLengthMemoryStream.cs Outdated
Comment thread src/tests/Refit.Tests/ThrowOnGetLengthMemoryStream.cs Outdated
Comment thread src/tests/Refit.Tests/ThrowOnGetLengthMemoryStream.cs Outdated
@glennawatson
glennawatson force-pushed the rethrow-cancellation-exception-while-buffering branch from 5594252 to 49d3174 Compare August 1, 2026 14:43
@glennawatson

Copy link
Copy Markdown
Contributor

Thanks for this - the bug is real. I have rebased onto latest main and pushed the review points to your branch, keeping you as the author.

What changed from your version:

  • The fix is now cancellationToken.ThrowIfCancellationRequested() after the existing catch, instead of a filtered catch/throw. Your version only worked on net9.0+, because the LoadIntoBufferAsync(CancellationToken) polyfill in src/Shared/HttpContentExtensions.cs drops the token on everything below, so buffering there never throws and the bug still shipped on net8.0, net4xx and netstandard2.0. It also broke SendAsyncRethrowsCancellationRequestedDuringDeserialization, which is what turned CI red.
  • The regression test is replaced. I checked the original one against unpatched product code and it passed, so it was not guarding anything - once buffering swallowed the cancellation, the stub stream threw the same exception from the serializer instead. The new SendAsyncStopsBeforeDeserializationWhenCancelledDuringBuffering uses the existing RecordingContentSerializer and asserts DeserializeCallCount == 0, so it fails if the serializer is ever reached. Verified red against main's RequestExecutionHelpers.cs and green with the fix.
  • Because the fix works on every target, the test needs no #if NET9_0_OR_GREATER, and the blocking-read machinery added to ThrowOnGetLengthMemoryStream is no longer needed, so that file and ResponseTests.cs are back to their original state. That also drops the SuppressMessage and the second responsibility on a class named for one behaviour.
  • The breaking-changes note moved to its own V15.x.x section with a TOC entry, rather than being appended to the V14 list.

Result is 58 added lines across 3 files. Solution builds clean on all ten target frameworks, all 7505 tests pass, and RequestExecutionHelpers.cs keeps 100 percent line and branch coverage.

@glennawatson
glennawatson enabled auto-merge (squash) August 1, 2026 14:54
@glennawatson
glennawatson merged commit 69ee15f into reactiveui:main Aug 1, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants