Fix SocketsHttpHandler canceling request when response stream dropped (dotnet/corefx...
authorStephen Toub <stoub@microsoft.com>
Mon, 15 Jul 2019 17:12:27 +0000 (13:12 -0400)
committerGitHub <noreply@github.com>
Mon, 15 Jul 2019 17:12:27 +0000 (13:12 -0400)
commit920886e407462c210930228512f13533246b20df
tree6ebef5d09bfba7bca0e3ba3ce87c6c4ef71cdc19
parent7adb86b94d28bdb43a3cf5784275fa63e84bd5ea
Fix SocketsHttpHandler canceling request when response stream dropped (dotnet/corefx#39471)

* Fix SocketsHttpHandler canceling request when response stream dropped

Good code using HttpClient should Dispose of the HttpResponseMessage and/or response Stream when its done with it.  However, if code fails to do so, we still want to ensure that we don't leak connections/requests to the server or leak resources on the client.  If the response isn't disposed but the code reads all the way to the end of it, then internally things are cleaned up.  But if the response isn't disposed and hasn't read to the end, for both HTTP/1.1 and HTTP/2, we have some issues.

For HTTP/1.1, we rely on the Socket's finalization to close the associated connection.  However, we use a cache of SocketAsyncEventArgs instances to handle creating connections, and it turns out that these SocketAsyncEventArgs instances are keeping around a reference to the last created Socket.  If the SAEA is reused for another connection, then there's no issue, but until it is, it maintains several references to the socket: in ConnectSocket, in _currentSocket, in _multipleConnect, and then further within another SAEA instance that the _multipleConnect itself is creating for some reason.  We should go through and clean all that up in SAEA, but for now, this just removes the SAEA cache.

For HTTP/2, nothing was cleaning up in this case.  To handle it, we:
1. Make the response stream finalizable, such that when finalized it behaves as if Dispose were called.
2. When SendAsync completes, we remove the strong reference from the Http2Stream to the response message, such that the response (and in turn the response stream) doesn't get rooted by the Http2Connection storing the Http2Stream in its dictionary).
3. The only reason that reference was still needed was to support trailing headers.  So we create a lazily-allocated list on the Http2Stream for storing trailers if any should arrive, and then when the response body completes, the response stream pulls those headers in from the temporary collection into the response that it then has a reference to.  This also helps to avoid a race condition where consuming code erroneously accesses TrailingHeaders before the request has completed, in which case we previously could have been trying to write to the collection while user code was reading from it; with this change, that mutation happens as part of a call the consumer makes.

* Address PR feedback

Commit migrated from https://github.com/dotnet/corefx/commit/8b5eb8d9fb7857e5b40e763287f8f8832c15af31
src/libraries/Common/tests/System/Net/Http/GenericLoopbackServer.cs
src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs
src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs [new file with mode: 0644]
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj