Linux HttpClient.SendAsync resets Content Stream position

[aviviadi]

In the past the stream was disposed after SendAsync call. Now it isn't : dotnet/corefx#19082

However, on Linux the Stream.Position is reset to zero. On windows it remains on the last read position.

The following program, i.e., set the MemoryStream position to last read position 3 in Windows, but to 0 in Linux

OS : Windows 10 vs. Ubuntu16.04
.NetCore 2.0.0

using System;
using System.IO;
using System.Net;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;

namespace StreamShouldNotBeReset
{
    internal class Program
    {
        internal class MyContent : HttpContent
        {
            private readonly Stream _stream;
            private readonly byte[] _buffer = new byte[4096];

            internal MyContent(Stream ms)
            {
                _stream = ms;
            }

            protected async Task CopyToStreamAsync(Stream stream)
            {
                var count = await _stream.ReadAsync(_buffer, 0, _buffer.Length).ConfigureAwait(false);
                while (count > 0)
                {
                    await stream.WriteAsync(_buffer, 0, count).ConfigureAwait(false);
                    count = await _stream.ReadAsync(_buffer, 0, _buffer.Length).ConfigureAwait(false);
                }
            }

            protected override Task SerializeToStreamAsync(Stream stream, TransportContext context)
            {
                return CopyToStreamAsync(stream);
            }

            protected override bool TryComputeLength(out long length)
            {
                if (_stream.CanSeek)
                {
                    length = _stream.Length;
                    return true;
                }

                length = 0;
                return false;
            }

            protected override Task<Stream> CreateContentReadStreamAsync()
            {
                return Task.FromResult(_stream);
            }
        }

        internal static async Task RequestHandler(HttpContext context)
        {
            var result = await context.Request.ReadFormAsync().ConfigureAwait(false);
            foreach (var keyValuePair in result)
            {
                Console.WriteLine(keyValuePair.Key + "=" + keyValuePair.Value);
            }
            context.Response.StatusCode = (int)HttpStatusCode.OK;
        }

        internal class MyStartUp
        {
            public void Configure(IApplicationBuilder app, ILoggerFactory loggerfactory)
            {
                app.Run(RequestHandler);
            }
        }

        private static void Main()
        {
            const string url = "http://127.0.0.1:8080";

            var host = new WebHostBuilder()
                .UseKestrel()
                .UseStartup<MyStartUp>()
                .UseUrls(url).Build();

            host.Start();

            using (var client = new HttpClient(new HttpClientHandler()))
            using (var ms = new MemoryStream(new byte[] {1, 2, 3}))
            {
                client.BaseAddress = new Uri(url);
                client.SendAsync(new HttpRequestMessage
                {
                    Method = HttpMethod.Put,
                    Content = new MyContent(ms)
                }).Wait();

                Console.WriteLine(ms.Position);

            }
        }
    }
}

[EDIT] Add C# syntax highlighting by @karelz

[davidsh]

cc: @stephentoub

[karelz]

@rmkerr can you please finish this work for 2.1? Thanks!

[karelz]

After a discussion we decided to move it to Future. We do not allow HttpRequestMessage reusing. Checking its position after it has been sent seems like a corner case, that does not block important scenarios.

[stephentoub]

We do not allow HttpRequestMessage reusing.

Not with HttpClient. What about with HttpMessageInvoker? Is it documented somewhere that the same message can't be used multiple times with it?

(To be clear, I'm not objecting to pushing this out; I just want to make sure our rationale is correct.)

[karelz]

@davidsh can you please chime in?

[rmkerr]

I don't see anything stating that we can't reuse an HttpRequestMessage with HttpMessageInvoker, but I think that is more of a documentation issue. In this case the issues that come up with request re-use are all in CurlHandler, and so are problematic either way.

[davidsh]

Is it documented somewhere that the same message can't be used multiple times with it?

It is not documented either way. However, in practice our implementation have evolved to be problematic for this.

However, this particular issue here is actually not about whether or not a dev can re-use an HttpRequestMessage object for additional SendAsync() calls. Instead, this issue is about a difference in behaviors between the Windows and Linux HTTP stacks. Specifically, the Windows HTTP stack leaves request body context objects (HttpRequestMessage.Content) as-is after being "consumed" by sending. I.e. it doesn't explicitly reset the stream pointer of an StreamContent object after SendAsync() is done. The Linux implementation is rewinding the StreamContent object for some reason after sending.

While it is true that the HttpClient stack will rewind an HttpContent (StreamContent) object during re-submits within a single SendAsync() (i.e. to resubmit due to 401, 3xx etc.), it won't do a final rewind of the content object. In general, the design principle in HttpClient/HttpContent objects is that they are "consumed" during send.

[rmkerr]

I'd like to add on to this -- what I found was that because of the limitations of CurlHandler and of HttpRequestMessage, it is difficult to make the stream rewind behavior consistent.

The longer explanation is that ContentLength gets cached in HttpContentHeaders and this causes problems in Curl if we don't rewind the stream. When we try to resend an HttpRequestMessage (unsupported, but as is it more or less works) the ContentLength has a cached non-zero value, but the actual content has all been read. This causes Curl to hang for several minutes before eventually crashing. That is the behavior that caused the the SendSameRequestMultipleTimesDirectlyOnHandler test to fail after the proposed change:
https://github.com/dotnet/corefx/blob/8e88850940605ef7d57cfc7b570bdf9bf2b8d822/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L2520-L2553

I think we should ideally converge the behavior at some point, but not for 2.1. That is especially true since we consider the HttpContent to have been consumed during send. It's something I'd be interested in working on later, but for now it would be a lot of effort spent on an unsupported behavior.

[stephentoub]

The longer explanation is that ContentLength gets cached in HttpContentHeaders and this causes problems in Curl if we don't rewind the stream.

I don't understand this explanation. The test you highlighted is passing on Windows. That means WinHttpHandler must be rewinding the stream as well to the starting position, as otherwise it would be failing this test, where the second iteration expects all of the relevant parts of the stream to be sent again, and that can only happen if the stream's position was rewound.

What am I missing?

[rmkerr]

You're right -- I definitely misunderstood the behavior on windows. In that case it may be worth giving this another look.

[stephentoub]

The issue has to do with StreamContent. There are two ways of getting out the stream:

• Via SerializeToStreamAsync, which is used by HttpContent.CopyToAsync
• Via CreateContentReadStreamAsync, which is used by HttpContent.ReadAsStreamAsync

When a StreamContent is created, its ctor stores the stream's Position. SerializeToStream rewinds the stream to that Position; CreateContentReadStreamAsync does not. And WinHttpHandler uses CopyToAsync; CurlHandler uses ReadAsStreamAsync. Hence the difference in behavior here. (HttpContent also caches the stream it gets back from CreateContentReadStreamAsync, so even if CreateContentReadStreamAsync were changed to restore the position, that alone wouldn't help).

[davidsh]

@karelz @stephentoub Does this problem occur with SocketsHttpHandler as well? I think this problem is only with CurlHandler. Since SocketsHttpHandler is the default for .NET Core 2.1 and we plan to deprecate CurlHandler, perhaps we can close this issue?

[stephentoub]

Does this problem occur with SocketsHttpHandler as well?

It shouldn't; SocketsHttpHandler uses the same CopyToAsync API that WinHttpHandler did, as outlined in https://github.com/dotnet/corefx/issues/23782#issuecomment-375790899.

perhaps we can close this issue?

We should add a test for this (or confirm we already have one), validate that it passes on HttpClientHandler now by default on both Windows and Unix, and then I think it's fine to close this.