Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix SocketsHttpHandler canceling request when response stream dropped#39471

Merged
stephentoub merged 2 commits intodotnet:masterfrom
stephentoub:dropstreams
Jul 15, 2019
Merged

Fix SocketsHttpHandler canceling request when response stream dropped#39471
stephentoub merged 2 commits intodotnet:masterfrom
stephentoub:dropstreams

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Jul 14, 2019

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.

cc: @geoffkizer, @davidsh, @wfurt, @scalablecory, @eiriktsarpalis
Fixes https://github.com/dotnet/corefx/issues/39470
Fixes https://github.com/dotnet/corefx/issues/39427
Fixes https://github.com/dotnet/corefx/issues/39420
Related to https://github.com/dotnet/corefx/issues/39466
Related to https://github.com/dotnet/corefx/issues/39461 (this doesn't fix that but will be impacted by it)

@geoffkizer
Copy link

Some minor feedback above, generally LGTM

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.
@stephentoub
Copy link
Member Author

/azp run

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 15, 2019
@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@stephentoub stephentoub merged commit 8b5eb8d into dotnet:master Jul 15, 2019
@stephentoub stephentoub deleted the dropstreams branch July 15, 2019 17:12
@karelz karelz added this to the 3.0 milestone Jul 16, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…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 dotnet/corefx@8b5eb8d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

4 participants