HttpClient ignores TransferEncodingChunked = false if content doesn't specify content length

[brandondahler]

Problem

When using HttpClient, even when TransferEncodingChunked is explicitly set to false, the underlying HttpMessageHandlers will force chunked content submission if the content does not explicitly define its Content-Length header.

This coupled with the default behavior changing from non-chunked to chunked from .Net Framework to .Net Core can result in a significantly harder barrier to converting existing applications. Working around this is possible by manually setting the Content-Length header; however, this affects other libraries that are built upon this infrastructure in ways that require you to bypass those library's benefits. Specifically see aspnet/AspNetWebStack#232 .

Reproduction Steps

Insert the following snippet into src\System.Net.Http\tests\FunctionalTests\HttpClientHandlerTest.cs and execute the test.

        [Fact]
        public async Task PostAsync_TransferEncodingChunkedFalse_DoesNotSendChunked()
        {
            const string ExpectedContent = "Hello, expecting and continuing world.";
            var clientCompleted = new TaskCompletionSource<bool>();
            await LoopbackServer.CreateClientAndServerAsync(async uri =>
            {
                using (HttpClient client = CreateHttpClient())
                {
                    client.DefaultRequestHeaders.TransferEncodingChunked = false;

                    var stringContent = new StringContent("test");
                    stringContent.Headers.ContentLength = null;

                    await client.PostAsync(uri, stringContent);
                    clientCompleted.SetResult(true);
                }
            }, async server =>
            {
                await server.AcceptConnectionAsync(async connection =>
                {
                    List<string> headers = await connection.ReadRequestHeaderAsync();
                    Assert.DoesNotContain("Transfer-Encoding: chunked", headers);

                    await connection.SendResponseAsync(content: ExpectedContent);
                    await clientCompleted.Task; // make sure server closing the connection isn't what let the client complete
                });
            });
        }

Expected Results

Transfer-Encoding: chunked is not set in the request sent to the server, Content-Length header is calculated and submitted instead.

Actual Results

Chunked encoding is used to submit the content of unknown length.

Discussion

It appears the purpose of the original change from non-chunked to chunked default is a result of the underlying HttpClientHandler implementations. From what I can tell, this was done for performance reasons -- by using chunked encoding, we can stream content to the server instead of having to buffer the entire response. I personally believe that the change in default makes sense and the positives outweigh the negatives; however, I believe we also inadvertently broke the ability to force non-chunked transfer encoding.

I believe this is significant because, while against HTTP 1.1, there are existing HTTP endpoints that are not able to properly handle chunked encoding. While I wouldn't expect us to necessarily cater to non-conforming HTTP servers, I do believe that we should also do our best effort to allow end users to force the less-performant method for non-compliant endpoints.

In a similar vein, I believe that the fact that there is the ability to set TransferEncodingChunked = false makes the end users believe that it would force non-chunked encoding.

Underlying Cause

I was able to track down the cause of this issue to the following code, at least for the SocketsHttpHandler.
https://github.com/dotnet/corefx/blob/063e38c7bc66e076fdcc7521e4c69650baae9427/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs#L368-L372

In this code we're not checking if request.Headers.TransferEncodingChunked.HasValue is false before forcing it to be true when ContentLength is null.

Possible Solutions

I believe one of these solutions would better serve the end users of the HttpClient class:

1. Update HttpClientHandler implementations to verify .TransferEncodingChunked.HasValue == false before forcing chunked transfer encoding. If TransferEncodingChunked.HasValue && !TransferEncodingChunked.Value then we should buffer the content, determine the final content length, set ContentLength to that value (so that it can be sent to the server properly), and finally use the buffered stream instead of re-fetching the content from the HttpContent instance.
2. Deprecate TransferEncodingChunked and rename to ForceTransferEncodingChunked that is defaulted to false.

[brandondahler]

Upon looking a little farther, this also means that you technically cannot use HttpContent that does not define an explicit Content-Length header if you use HTTP 1.0 -- you will necessarily get an exception where you wouldn't have gotten one in .NET Framework.

https://github.com/dotnet/corefx/blob/063e38c7bc66e076fdcc7521e4c69650baae9427/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs#L368-L380

[davidsh]

@stephentoub @geoffkizer

[geoffkizer]

It's not clear to me whether this is actually an issue in practice or not. I haven't seen any previous reports on it. If it's actually affecting customer scenarios, we could look at buffering as you suggest.

[brandondahler]

I can't say I necessarily know that is a problem in practice or not, but I'd be willing to make an open source contribution to resolve it if you're interested. I was originally forwarded aspnet/AspNetWebStack#232 by a coworker thinking their problem was related (it wasn't), but I was interested in understanding why DefaultRequestHeaders.TransferEncodingChunked = false didn't work for @chkeita in this comment aspnet/AspNetWebStack#232 (comment) .

I think regardless we ought to work towards one of the solutions so that its less deceptive in that TransferEncodingChunked is a nullable bool but setting it to false is effectively a no-op.

[geoffkizer]

If you want to submit a PR, you're welcome to. Contributions are definitely appreciated.

Before getting too far with the actual code change, though, we should agree on the desired behavior. How are you proposing this should work?

[geoffkizer]

Looking at your "possible solutions" from the original post:

(1) seems reasonable to me. I assume you are also proposing something re HTTP/1.0 here. Specifically, that in the 1.0 case, we never implicitly set TE for you, and thus if you don't have a Content-Length then we will end up in this new case where we buffer.

This would result in two changes in behavior, as I can see it:
(a) If you use HTTP/1.1 and explicitly set TEChunked=false, and you don't have a Content-Length, today we will send TE: chunked anyway. With this change, we would instead buffer and send Content-Length. This seems reasonable to me. It's rare to set TEChunked=false explicitly, and if you do this, then we should respect it.
(b) If you use HTTP/1.0 and don't provide a Content-Length, today we fail. With this change, we would instead buffer and succeed. This seems like goodness.

Re your option (2): it would be better to avoid adding new API here unless there's a really good reason to do so.

[geoffkizer]

BTW, for doing the actual buffering, we should be able to just use HttpContent.LoadIntoBufferAsync, I believe.

[scalablecory]

As noted in dotnet/corefx#40695, this is a quirk we think we will need to live for compatibility and predictability reasons. Luckily, this seems to be an edge case that most people will not hit. For now, we should focus on updating our documentation to make note of this behavior.

[brandondahler]

Fair, want to leave this issue open for documentation purposes? Otherwise I'll go ahead and close it out.

[geoffkizer]

I'll copy the comment I made on the PR here, for posterity:

If you really want to send using CL instead of TE, and you have content that doesn't provide CL, then you need to call LoadIntoBufferAsync before using the content. This will cause us to send with CL (unless you explicitly set TEChunked=true).