ExpectContinue causes breaking behaviour in responseStream.Read

[peppy]

Since 2.1.300, setting ExpectContinue to true will cause Read operations on the response stream to block for a long period rather than immediately returning 0 at the end of stream content.

Reproduction:

using System;
using System.IO;
using System.Net;
using System.Net.Http;

namespace http_response_read_test
{
    class Program
    {
        private const int buffer_size = 2048;

        static void Main(string[] args)
        {
            var client = new HttpClient();

            client.DefaultRequestHeaders.ExpectContinue = true;

            var response = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "https://httpbin.org/get")).Result;
            var buffer = new byte[buffer_size];

            using (var responseStream = response.Content.ReadAsStreamAsync().Result)
            {
                int read;
                while ((read = responseStream.Read(buffer, 0, buffer_size)) > 0)
                {
                    // consume buffer
                }
            }
            
            System.Console.WriteLine("finished");
        }
    }
}

Running this code under netcoreapp2.0 and net471 will exit immediately after the response is read, while running under netcoreapp2.1 will block for a very long time before eventually exiting (takes around a minute).

Commenting out the ExpectContinue line will cause all three runtimes to match in behaviour.

[stephentoub]

The issue here is the client is dutifully sending the Expect: 100-continue header you asked for, but there is no content (you haven't added any content to the HttpRequestMessage). The server ends up sending back a 100 status code response and then doesn't send back the rest of the response because the server is waiting for content that will never come, and the client is waiting for that response content; eventually the server times out and sends the response and the client read completes. Note, too, that the timeout on the server side differs from server to server; https://httpbin.org apparently has a really long timeout, but switch to https://microsoft.com, for example, and the repro completes quickly.

netcoreapp2.0 doesn't support ExpectContinue at all on Windows (it's just ignored), which is why it doesn't manifest there. And .NET Framework doesn't support sending request content with the GET verb (try it and it'll throw an exception), so it won't send Expect: 100-continue with GET because it won't be sending any content.

You have a variety of ways to address this, e.g.

1. Don't set ExpectContinue = true; if you're not going to be sending request content.
2. Send request content, e.g. add { Content = new ByteArrayContent(Array.Empty<byte>()) } when constructing the HttpRequestMessage.

@davidsh, @geoffkizer, if a dev sets ExpectContinue = true, should we explicitly suppress the header if there is no request content? It feels a bit wrong to me to ignore the dev's explicit request, but it's also non-sensical if there's no request content.

[davidsh]

@davidsh, @geoffkizer, if a dev sets ExpectContinue = true, should we explicitly suppress the header if there is no request content? It feels a bit wrong to me to ignore the dev's explicit request, but it's also non-sensical if there's no request content.

On .NET Framework, ServicePointManager.Expect100Continue defaults to TRUE. But I don't think it sends that header if there is no content to send. So, we should refrain from sending that request header is there is no content.

[stephentoub]

Doesn't it seem a bit odd to explicitly suppress a header the developer has asked to be sent?

Note that today WinHttpHandler, CurlHandler, and SocketsHttpHandler all fail this test, which to me seems correct (they all send the header that was requested by the dev):

[Fact]
public async Task GetAsync_ExpectContinueTrue_NoContent_NoExpectHeaderSent()
{
    await LoopbackServer.CreateClientAndServerAsync(async uri =>
    {
        using (var client = CreateHttpClient())
        {
            client.DefaultRequestHeaders.ExpectContinue = true;
            await client.GetStringAsync(uri);
        }
    }, async server =>
    {
        List<string> headers = await server.AcceptConnectionSendResponseAndCloseAsync();
        Assert.DoesNotContain("Expect: 100-continue", headers);
    });
}

[davidsh]

Doesn't it seem a bit odd to explicitly suppress a header the developer has asked to be sent?

I guess it depends on what "explicitly" means.

On .NET Framework, the property defaults to TRUE. Or they can set it to TRUE (explicitly). Still, there is no failure. They could also probably add the header manually on .NET Framework and I still think it is ignored if there is no content.

.NET Core defaults to FALSE. So, perhaps the dev knows what they are doing. Maybe we can keep the header there but emit an EventSource warning/error about this when they do it with no Content object.

[stephentoub]

Oops... bug in the test...

[stephentoub]

Oops... bug in the test...

Fixed. And same outcome... all three handlers currently send Expect: 100-continue when asked to.

.NET Core defaults to FALSE. So, perhaps the dev knows what they are doing. Maybe we can keep the header there but emit an EventSource warning/error about this when they do it with no Content object.

That sounds reasonable to me.

[peppy]

Thanks for the response. I did suspect it was an issue due to how we were using ExpectContinue, but did feel it strange that behaviour changed between versions.

[stephentoub]

.NET Core defaults to FALSE. So, perhaps the dev knows what they are doing. Maybe we can keep the header there but emit an EventSource warning/error about this when they do it with no Content object.

@davidsh, alternatively, what would you think about sending a Content-Length: 0 header when Expect: 100-continue is set but no content was specified. For non-GET/HEAD verbs, we already add that header if Content == null; we could just also do so for those verbs if Expect: 100-continue is set.

[davidsh]

@davidsh, alternatively, what would you think about sending a Content-Length: 0 header when Expect: 100-continue is set but no content was specified. For non-GET/HEAD verbs, we already add that header if Content == null; we could just also do so for those verbs if Expect: 100-continue is set.

I would be ok w/ that.

[stephentoub]

Actually, mea culpa, I dug into this deeper, and the issue isn't what I said it was. I believe I now understand what's going on and will work on a fix.

[stephentoub]

Fixed in master

[stephentoub]

Fixed in 2.1.3 in PR dotnet/corefx#30112.