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 Jul 15, 2019
Merged
Fix SocketsHttpHandler canceling request when response stream dropped#39471stephentoub merged 2 commits intodotnet:masterfrom
stephentoub merged 2 commits intodotnet:masterfrom
Conversation
geoffkizer
reviewed
Jul 15, 2019
geoffkizer
reviewed
Jul 15, 2019
geoffkizer
reviewed
Jul 15, 2019
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Show resolved
Hide resolved
geoffkizer
reviewed
Jul 15, 2019
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
geoffkizer
reviewed
Jul 15, 2019
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Show resolved
Hide resolved
|
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.
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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)