Always read from underlying stream in StreamPipeReader #118041
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue in StreamPipeReader where the reader would incorrectly cache that the underlying stream had reached its end, preventing it from re-reading data when the stream position was reset. The change removes internal state tracking of stream completion and always attempts to read from the underlying stream when there's no unexamined buffered data.
- Removes the
_isStreamCompletedfield that was preventing subsequent reads after the stream reached EOF - Updates read logic to determine completion status dynamically rather than caching it
- Modifies test cases to use standard
MemoryStreaminstead of custom test streams and adds a new test for stream position reset scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/StreamPipeReader.cs | Removes _isStreamCompleted field and associated logic that cached stream completion state |
| src/libraries/System.IO.Pipelines/tests/StreamPipeReaderTests.cs | Updates existing tests and adds new test case for stream position reset behavior |
src/libraries/System.IO.Pipelines/tests/StreamPipeReaderTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
This would mean wrapping a networkstream would now throw after the EOF is read? (As it'll transition to non-readable, causing an exception on the next ReadAsync). EDIT: nevermind, this won't happen until Close is called. |
|
Right, most Streams should continue working until Close/Dispose is called on them. |
|
I’m remembering this was added for a reason …. |
Was it a good reason? 😆 |
|
Would checking |
|
I know about Chesterton's Fence, but there are a lot of apps that use |
|
/backport to release/10.0 |
|
Started backporting to |
…#122670) Backport of #118041 to release/10.0 /cc @BrennanConroy ## Customer Impact - [x] Customer reported - [x] Found internally Reported in dotnet/aspnetcore#64323. Customers using request buffering (or similar seekable streams) and reading through the request before framework/app code processes the request are now getting empty data or errors in the app/framework code. The expectation is that if using a seekable stream and doing some (pre-)processing of the request before letting application code see the request, then the application code should be able to read from the stream and get the full request as if no (pre-)processing occurred. ## Regression - [x] Yes - [ ] No `StreamPipeReader` didn't regress, but ASP.NET Core used it in more places in 10.0 which caused a regression in ASP.NET Core. dotnet/aspnetcore#62895 added `PipeReader` support for Json deserialization to most of ASP.NET Core's json parsing code which meant the `StreamPipeReader` ended up getting used during json deserialization where previously it was using the `Stream`. ## Testing Verified that the original issues described in dotnet/aspnetcore#64323 were fixed with this change. (both MVCs `IAsyncResourceFilter` and Minimals `IEndpointFilter`) Also verified skipped test https://github.com/dotnet/aspnetcore/blob/1a2c0fd99c05db9f9ef167165386d14d3e24c9b4/src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.BindAsync.cs#L257-L258 was fixed by this change. ## Risk Low; This could cause a regression if someone was reading from a `Stream` after it had returned `0` (exceptions below). This is not a common pattern as most code would exit the read loop or be done processing the `Stream` at that point. Scanned a few dozen `Stream` implementations to verify that calling read after a previous read returned 0 does not throw. Most code that calls read after a previous read returned `0` are either seeking (rewinding) or doing zero-byte reads. --------- Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
As part of dotnet/aspnetcore#62895 we realized that
StreamPipeReaderwas internally storing the fact that the underlyingStreamreturned 0 indicating end of stream. This causes problems with code that might modify theStream.Positionafter theStreamwas completely read from. Meaning future read calls onStreamPipeReaderwill always complete without re-reading theStream.This behavior doesn't seem necessary so relaxing the restrictions to always read from the underlying
Stream, assuming thePipeReaderisn't completed and there isn't any unexamined buffered data.