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

HttpContent auto-computed content length header is not exposed via header enumeration #25086

Closed
geoffkizer opened this issue Feb 19, 2018 · 5 comments

Comments

@geoffkizer
Copy link
Contributor

If you access HttpContent.Headers.ContentLength directly, we will attempt to use the content's TryComputeLength to construct a content length header.

However, if you access HttpContent.Headers via its enumerator or a method like TryGetValues, we won't construct the content length header.

This means that just accessing the ContentLength property will change the results of header enumeration. For example:

            var content = new StringContent("hello world");
            foreach (var kvp in content.Headers) { }    // Content-Length not present
            var ignore = content.Headers.ContentLength;
            foreach (var kvp in content.Headers) { }    // Content-Length present

Similarly, if you try to remove the header using the Remove method, it's not actually removed:

            var content = new StringContent("hello world");
            content.Headers.Remove("Content-Length");
            Assert.Equal(null, content.Headers.ContentLength);  // fails; returns value from TryComputeLength

The easiest fix here is probably just to try to compute the length when the content object is constructed. This assumes that TryComputeLength is always cheap, which seems reasonable, but is slightly different than how it's used today. Also, it seems like LoadIntoBufferAsync changes the Content-Length, but probably this is fine.

Unfortunately, fixing this will cause SocketsHttpHandler to behave incorrectly. Currently, if you set TransferEncodingChunked = true, then SocketsHttpHandler will never access the ContentLength property directly, and thus it won't get populated and won't get sent on the wire. Fixing the above behavior will cause the ContentLength to get populated, and thus sent on the wire, which is not correct behavior, per RFC. So this will need to get fixed somehow. (Note that this problem exists today, it's just harder to hit it -- you would need to either set Content-Length explicitly, or force population of the header by accessing it.)

(edited because I'm an idiot and can't read the RFC properly)

@stephentoub
Copy link
Member

@davidsh
Copy link
Contributor

davidsh commented Feb 19, 2018

This is a known bug and we decided at the time not to fix it due to app-compat concerns.

Duplicate of https://github.com/dotnet/corefx/issues/5523

Internal bug 122111.

cc: @Tratcher

[edit]

@geoffkizer
Copy link
Contributor Author

This also affects MultipartContent in a similar way to SocketsHttpHandler. If you access Content-Length on one of the content parts, it will end up getting serialized in that part's headers, which is probably not what you want.

@geoffkizer
Copy link
Contributor Author

I've submitted a PR for the SocketsHttpHandler fix. That's the most pressing issue here.

I suppose we can continue to live with the current behavior. That said, this is the sort of thing that causes subtle bugs, as demonstrated by the SocketsHttpHandler issue.

@davidsh
Copy link
Contributor

davidsh commented Apr 16, 2018

Closing this bug as duplicate of #16162

@davidsh davidsh closed this as completed Apr 16, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants