Reject empty Slice segment in the middle of an element stream#4756
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in Slice element-stream decoding where a zero-size Slice segment occurring mid-stream was incorrectly treated as end-of-stream, leading to silent element loss (and a Debug.Assert in debug builds). The stream enumerator now distinguishes a legitimate trailing empty segment (end-of-stream) from an empty segment that is not completed (protocol violation).
Changes:
- Update
AsyncStream<T>.EnumerateAsyncto throwInvalidDataExceptionwhen it encounters an empty segment that does not also mark stream completion. - Add a regression test ensuring a mid-stream zero-size segment throws while still yielding already-decoded elements and completing the reader exactly once.
- Add a test covering the legal case where a trailing zero-size segment ends iteration normally.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/IceRpc.Slice/Operations/Internal/AsyncStream.cs | Rejects empty, non-completed segments mid-stream by throwing InvalidDataException instead of silently ending iteration. |
| tests/IceRpc.Slice.Tests/AsyncStreamTests.cs | Adds tests for both the invalid mid-stream zero-size segment case and the valid trailing zero-size segment case; includes a helper to build segmented payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I think we should actually reject 0 segment sizes in general, in A Slice segment with an encoded 0-size is not correct:
The "0" case is the end of stream; we don't encode a size 0. Note that we currently have various tests that encode a 0 Slice segment. They'll need to be updated. |
| { | ||
| // A zero-size segment is never valid: the end of a stream is signaled by the reader completing with no | ||
| // bytes, not by a zero-size segment. See https://github.com/icerpc/icerpc-csharp/issues/4755. | ||
| byte[] payload = EncodeInt32Segments([1, 2], [], [3]); |
There was a problem hiding this comment.
This doesn't look like a good test: a stream of int32 is not encoded in segments. It would be better to pick a variable-size element like a string.
There was a problem hiding this comment.
🤖 Claude: Done in 415bc1b. Converted both zero-size-segment tests to stream a variable-size element (string) via the segment-based ToAsyncStream overload. You're right that this matters: fixed-size elements like int32 use the elementSize overload, whose ReadAsync calls ReadAtLeastAsync and never reads segments — so a zero-size segment can only occur in a variable-size element stream. The previous int32 + no-elementSize pairing tested a combination that does not happen on the wire. Verified the new string tests fail against the unfixed IsCompleteSegment and the full Slice + IceRpc suites pass.
| { | ||
| // A zero-size segment is never valid, including at the end of the stream: the end of a stream is signaled | ||
| // by the reader completing with no bytes, not by a zero-size segment. | ||
| // See https://github.com/icerpc/icerpc-csharp/issues/4755. |
There was a problem hiding this comment.
🤖 Claude: Done in 415bc1b — same change applied to this test (now streams string). See the reply above for the rationale.
Per review feedback: only variable-size element streams are encoded with segments (fixed-size elements like int32 use the elementSize overload, which reads fixed chunks without segment framing). The zero-size segment tests now stream strings so they exercise a scenario that can actually occur on the wire.
Fixes #4755.
AsyncStream<T>.EnumerateAsynctreated any empty buffer as the end of the stream. A zero-size Slice segment followed by more data also produces an empty buffer withIsCompletedfalse (seePipeReaderExtensions.IsCompleteSegment): aDebug.Assertfired in debug builds, and in release builds the element stream ended silently, dropping the remaining elements.The only expected empty segment is the one that ends the stream.
EnumerateAsyncnow throwsInvalidDataExceptionwhen it reads an empty segment in the middle of an element stream.Changes:
AsyncStream.EnumerateAsync: distinguish "empty + completed" (end of stream) from "empty + not completed" (protocol violation,InvalidDataException).Zero_size_segment_in_the_middle_of_the_stream_throws_InvalidDataException: written first against the unfixed code, where it failed withDebug.Fail; also checks that elements decoded before the invalid segment are still yielded and that the reader is completed exactly once.Zero_size_segment_at_the_end_of_the_stream_completes_iteration: pins the legal case, a trailing zero-size segment ends the stream normally.The C# encoder (
AsyncEnumerablePipeReader) never emits zero-size segments, so this only affects streams received from a peer that does.