Cache canceled read results in SlicPipeReader#4766
Merged
Conversation
Fixes icerpc/icerpc-csharp-audit#30. ReadAsync and TryRead returned canceled read results without caching them in _readResult. AdvanceTo computes the consumed and examined offsets against _readResult.Buffer, so advancing a canceled read with positions from the returned buffer computed the offsets against a stale buffer (or the default read result when the first read is canceled): GetOffset either throws ArgumentOutOfRangeException or produces garbage offsets that corrupt the window accounting fed to the StreamWindowUpdate frames. Canceled read results are now cached like regular ones, so AdvanceTo operates on the buffer the application actually received. Also fix the ReadFrameAsync test helper to consume only one frame's bytes when several frames are buffered back to back.
There was a problem hiding this comment.
Pull request overview
This PR fixes a Slic transport correctness bug where canceled PipeReader read results (ReadAsync/TryRead) were not cached in SlicPipeReader, causing AdvanceTo to compute offsets against a stale buffer and potentially corrupt flow-control window accounting (or throw).
Changes:
- Cache canceled
ReadResultvalues inSlicPipeReader.ReadAsyncand.TryReadsoAdvanceToalways computes offsets against the buffer actually returned to the application. - Add regression tests covering cancel/advance/resume scenarios, including a wire-level invariant test validating
StreamWindowUpdateaccounting. - Fix the
ReadFrameAsynctest helper to consume/copy exactly one frame when multiple frames are buffered back-to-back.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/IceRpc.Tests/Transports/Slic/SlicTransportTests.cs | Adds new cancel/advance/resume tests and fixes ReadFrameAsync to read exactly one frame per call. |
| src/IceRpc/Transports/Slic/Internal/SlicPipeReader.cs | Caches canceled read results in _readResult to keep AdvanceTo offset computations consistent and prevent window-accounting corruption. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pepone
commented
Jun 17, 2026
pepone
commented
Jun 17, 2026
pepone
commented
Jun 17, 2026
bernardnormier
approved these changes
Jun 17, 2026
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 free
to 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 icerpc/icerpc-csharp-audit#30.
ReadAsyncandTryReadreturned canceled read results without caching them in_readResult.AdvanceTocomputes the consumed and examined offsets against_readResult.Buffer, so advancing a canceled read with positions from the returned buffer computed the offsets against a stale buffer — or the default read result when the first read is canceled:GetOffseteither throwsArgumentOutOfRangeExceptionor produces garbage offsets that corrupt the window accounting (_examined/_windowSize) fed to the StreamWindowUpdate frames. The repository's own conformance tests advance canceled results, but only in arrangements where the stale offsets happen to be benign, which is why this stayed latent.Canceled read results are now cached like regular ones, so
AdvanceTooperates on the buffer the application actually received.Tests
Canceled_read_with_buffered_data_then_resume_reading(ReadAsyncandTryReadvariants): cancels a read that returns buffered data, advances with positions from the returned buffer (examine all, consume nothing), and verifies reading resumes correctly through the end of the stream. Both variants fail without the fix withArgumentOutOfRangeExceptionfrom the stale-buffer offset computation.Canceled_read_advance_does_not_corrupt_window_accounting: wire-level test (raw duplex client) pinning the flow-control invariant that the sum ofStreamWindowUpdateincrements sent to the peer never exceeds the bytes written on the stream, across a cancel/advance/resume sequence. This one is invariant-pinning rather than regression-detecting: with the unfixed code its particular advance positions happen to produce benign offsets.ReadFrameAsynctest helper copied and consumed the whole buffered sequence into a frame-sized array, which throws (and would drop trailing frames) when several frames are buffered back to back — single-frame arrangements never hit this. It now slices exactly one frame per call, which the wire-level test relies on to read consecutive window updates.Full
IceRpc.Testssuite passes (990 passed / 0 failed),dotnet buildanddotnet format --verify-no-changesclean.