Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SetResponseOnHttpContext can throw - Improper use of content.Length #556

Closed
davidni opened this issue Aug 17, 2018 · 2 comments
Closed

SetResponseOnHttpContext can throw - Improper use of content.Length #556

davidni opened this issue Aug 17, 2018 · 2 comments
Labels
bug Identified as a potential bug help wanted Not actively being worked on. If you plan to contribute, please drop a note. merged Issue has been merged to dev and is waiting for the next release small effort Likely less than a day of development effort.

Comments

@davidni
Copy link
Contributor

davidni commented Aug 17, 2018

Expected Behavior

Proxying should work with a custom IHttpRequester

Actual Behavior

Proxying fails if the custom IHttpRequester returns a response whose content isn't wrapping a MemoryStream. Things work by default because HttpClient.SendAsync's default is to buffer the entire response into an internal MemoryStream, which supports Stream.Length.

However, if a custom IHttpRequester uses HttpClient.SendAsync(..., HttpCompletionOption.ResponseHeadersRead), the resulting content will wrap a non-seekable stream. This will throw if trying to read Stream.Length.

Culprit is HttpContextResponder.SetResponseOnHttpContext:

AddHeaderIfDoesntExist(context, new Header("Content-Length", new []{ content.Length.ToString() }) );

Steps to Reproduce the Problem

  1. Implement a custom IHttpRequester that calls HttpClient.SendAsync(..., HttpCompletionOption.ResponseHeadersRead)
  2. Make a request to a target that returns a response body

Specifications

  • Version: 10.0.2
  • Platform: Windows 10, .NET Framework 4.6.2
@TomPallister
Copy link
Member

@davidni what do you think the best fix is here? I guess I need a failing test first! But was thinking...

 if(response.Content.Headers.ContentLength != null)
            {
                AddHeaderIfDoesntExist(context, new Header("Content-Length", new []{ response.Content.Headers.ContentLength.ToString() }) );
            }

@TomPallister TomPallister added bug Identified as a potential bug help wanted Not actively being worked on. If you plan to contribute, please drop a note. small effort Likely less than a day of development effort. labels Aug 17, 2018
@TomPallister TomPallister added the merged Issue has been merged to dev and is waiting for the next release label Aug 25, 2018
@TomPallister
Copy link
Member

Should be fixed in 10.0.4! Let me know if any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug help wanted Not actively being worked on. If you plan to contribute, please drop a note. merged Issue has been merged to dev and is waiting for the next release small effort Likely less than a day of development effort.
Projects
None yet
Development

No branches or pull requests

2 participants