Fix HttpRequestMessage reuse handling in HttpClientHandler on Unix
authorStephen Toub <stoub@microsoft.com>
Fri, 24 Jun 2016 13:23:57 +0000 (09:23 -0400)
committerStephen Toub <stoub@microsoft.com>
Fri, 24 Jun 2016 13:23:57 +0000 (09:23 -0400)
commit50a73e17792aeaea1847c384c1b6e1e86c242565
tree31b7c6b0b653940972c62380f3dd928c8873fb4e
parent42726389975f6cf7a6a20de42456dcccc0c6f3ff
Fix HttpRequestMessage reuse handling in HttpClientHandler on Unix

HttpRequestMessages are meant to be used for a single request.  However, that checking is done by HttpClient rather than by HttpClientHandler.  As a result, if code bypasses HttpClient, e.g. by using HttpMessageInvoker or by using a custom HttpMessageHandler sitting between the HttpClient and the HttpClientHandler, the same HttpRequestMessage can be used multiple times without an exception being thrown warning of the invalid usage.  And on Windows, things happen to work, but with our Unix implementation was never expecting this scenario and doesn't work well with it:
- The handler is explicitly disposing of the request content stream rather than leaving that up to the request content's disposal to handle.
- The handler is removing the Content-Length header from the request message content, as a simple way to avoid overriding the Content-Length header that libcurl itself may send based on previous configuration.
- The handler is using ReadAsStreamAsync to get the request stream to send, but ReadAsStreamAsync caches the returned stream, so if the same request message is reused with the same Content, the stream will have already advanced to the end of the stream, and when the message gets reused, we won't have content to send.

This commit fixes that:
- We now don't dispose the request content stream.  That'll be left up to the request content's disposal.
- We now don't remove the ContentLength header from the request message.  We instead just check as we're writing out the headers whether the header is Content-Length, skipping it if it is.
- We now store the original position of a seekable stream that's provided to us, and at the end of the request, rather than disposing of the stream, we rewind to the original position if it exists.  (If the stream isn't seekable, reusing the request message wouldn't be possible anyway.)

This last point also fixes another theoretical issue.  When libcurl wants to go back to the beginning of the stream (e.g. in a redirect scenario where it needs to rewind), it asks us to seek back to the beginning.  We're currently seeking back to the exact position asked for.  But the stream may have come in to us already advanced beyond 0, in which case we need to seek back to the requested offset plus that original position rather than just the requested offset.

And in doing this, I noticed that we were inadvertently allowing a set of closure/delegate allocations due to inadvertently using some captured state in a place where we were trying to avoid that.  I fixed that as well.

We were also marking everything complete before we'd cleaned up all resources, which meant that any failures from such cleanup would go unnoticed.  I've fixed that to ensure we clean up all state before we officially declare the async operation done.

Commit migrated from https://github.com/dotnet/corefx/commit/650cb2698f02635ea69da3ec9637f6ad9e7229c5
src/libraries/System.Net.Http/src/System/Net/Http/Unix/CurlHandler.EasyRequest.cs
src/libraries/System.Net.Http/src/System/Net/Http/Unix/CurlHandler.MultiAgent.cs
src/libraries/System.Net.Http/src/System/Net/Http/Unix/CurlHandler.cs
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs