Fixing bug around request buffering #1373
Merged
Conversation
This file contains 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
A regression was introduced when support was added in #1352 to support scenario where body stream buffering was enabled. Unfortunately, during that PR, non-buffered streaming scenarios were not regression tested leading to a bug in that (the default) scenario. The bug was caused by setting `Position = 0` on the body stream when non-buffered because, non-buffered streams are not seekable and thus throw a `NotSupportedException` if you attempt to change their position.
drub0y
commented
Feb 8, 2019
| // In the case of request buffering being enabled, we could possibly receive a Stream that needs its position reset, | ||
| // but we can't _blindly_ reset in case buffering hasn't been enabled since we'll be working with a non-seekable Stream | ||
| // in that case which will throw a NotSupportedException | ||
| if (requestBody.CanSeek) |
There was a problem hiding this comment.
Only attempt to reposition the Stream when it's "seekable". CanSeek will be true in the buffered scenario and false in the non-buffered (default) scenario.
drub0y
commented
Feb 8, 2019
| @@ -97,6 +98,14 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env) | |||
| app.UseDeveloperExceptionPage(); | |||
| } | |||
|
|
|||
| // NOTE: Uncomment this to force request buffering to test accessing the request body in buffered scenarios (default is always unbuffered) | |||
There was a problem hiding this comment.
I added this inline middleware to the Test Web App project to facilitate the testing of the two scenarios. I'm leaving it here commented out for the default (non-buffered scenario) and easy uncommenting in the future if the buffered scenario need to be tested again.
Pull Request Test Coverage Report for Build 48793
💛 - Coveralls |
cleemullins
approved these changes
Feb 9, 2019
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 #1371
A regression was introduced when support was added in #1352 for a scenario where body stream buffering was enabled. Unfortunately, during that PR, non-buffered streaming scenarios were not regression tested leading to a bug in that (the default) scenario.
The bug was caused by setting
Position = 0on the body stream when non-buffered because, non-buffered streams are not seekable and thus throw aNotSupportedExceptionif you attempt to change their position.