[release/10.0] Always read from underlying stream in StreamPipeReader #122670
+39
−69
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.
Backport of #118041 to release/10.0
/cc @BrennanConroy
Customer Impact
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
StreamPipeReaderdidn'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 addedPipeReadersupport for Json deserialization to most of ASP.NET Core's json parsing code which meant theStreamPipeReaderended up getting used during json deserialization where previously it was using theStream.Testing
Verified that the original issues described in dotnet/aspnetcore#64323 were fixed with this change. (both MVCs
IAsyncResourceFilterand MinimalsIEndpointFilter)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
Streamafter it had returned0(exceptions below). This is not a common pattern as most code would exit the read loop or be done processing theStreamat that point.Scanned a few dozen
Streamimplementations to verify that calling read after a previous read returned 0 does not throw. Most code that calls read after a previous read returned0are either seeking (rewinding) or doing zero-byte reads.