Dispose of the inner HttpClient in HttpWebRequest #8470
Conversation
|
@davidsh can say for sure, but this doesn't look like a valid change to me. In particular, in some cases I'd think it must not dispose of the HttpClient as you've done, because the response stream needs to remain valid for consumption even after the async method completes. |
This is not needed. The contract design for HttpClient allows the HttpClient object to be disposed even though a particular HttpResponseMessage exists and might have data left to read from its response stream. However, you can not dispose of the HttpResponseMessage because it will then dispose all contained objects within. And that means dispose the .Content object which contains the StreamContent of the response stream. |
| var request = new HttpRequestMessage(new HttpMethod(_originVerb), _requestUri); | ||
|
|
||
| if (_requestStream != null) | ||
| using (var handler = new HttpClientHandler()) |
There was a problem hiding this comment.
It is not necessary to have a using for the handler. Disposing the HttpClient will dispose the inner handler automatically. So, it's only necessary to have the using for the HttpClient itself.
There was a problem hiding this comment.
@davidsh Fixed, thanks for pointing that out.
In that case we're not currently handling this according to that contract on Unix. When the HttpClient is disposed, we effectively end up canceling all outstanding operations. You're saying that's not the right behavior, correct? And instead all outstanding requests/responses should be allowed to complete, with HttpClient itself preventing further requests from being started, and then all resources being cleaned up once all outstanding requests have completed? |
Yes. It's clear from the test results that *Nix implementation is different from Windows as per this design constraint. It's not even a design of HttpClient object itself, per se, but rather the HttpClientHandler design. The handler might be disposed (if you dispose the HttpClient, the handler gets disposed) to prevent more requests but that shouldn't close down the open network connections from the existing response streams, etc. |
|
And the WinRT Windows.Web.Http.HttpClient works the same way. |
Yes, we have explicit code in there to forcibly shut down in the case of Dispose. Having the behavior match should just entail deleting that code, something I always enjoy ;) |
|
@jamesqo Thanks for working on this PR since it exposed something broken in our *Nix implementation in HttpClient. @stephentoub We probably need more tests to validate that disposing an HttpClient won't affect reading from existing HttpResponseMessage response streams. |
Yeah, I'll add one as part of fixing it. |
|
(It looks like Ubuntu and OSX PR legs are defined to use TestWithLocalLibraries=true, while CentOS is not. I believe that's why after my fix in #8484 the Ubuntu and OSX legs are now passing but the CentOS legs are not; those legs should start working once we update to packages that include the fix.) |
|
LGTM. Thanks, @jamesqo. |
|
LGTM. We'll merge when green. |
|
Test Innerloop CentOS7.1 Debug Build and Test please |
Dispose of the inner HttpClient in HttpWebRequest Commit migrated from dotnet/corefx@a67304d
While browsing through the code recently, I noticed that
HttpWebRequestuses anHttpClientinternally, which it does not dispose of afterwards. I made a quick fix to useusingwhere the variables were initialized; since they're not saved to fields or anything I'm assuming I don't have to touch the finalizer/Dispose methods of this class. (Also assuming that sinceclient.SendAsyncis awaited immediately after it is invoked, we don't have to worry about the client being disposed before it finishes sending or anything.)Changes:
usingstatementscc @davidsh @stephentoub