Stop disposing of request content in HttpClient #19082
Conversation
|
Netstandard libraries won't be able to take advantage of this behavior because of the differences. Hopefully we can make this the same on desktop with a quirk? |
Because the System.Net.Http NuGet package OOBs on Desktop, it will have this change. However, we should consider making an actual change to the inbox System.Net.Http.dll in .NET Framework as well. That would require a quirk and opt-out via AppSettings, etc. That change probably will not making the current .NET vNext though due to scheduling. |
| using (HttpResponseMessage response = await post) | ||
| { | ||
| Assert.True(contentDisposed, "Expected request content to be disposed"); | ||
| Assert.Equal(PlatformDetection.IsFullFramework, contentDisposed); |
There was a problem hiding this comment.
This test is always going to fail. Since System.Net.Http OOBs on Desktop, these tests never actually test inbox System.Net.Http.dll. They test the OOB on Desktop.
There was a problem hiding this comment.
Comment applies to other tests in this PR.
There was a problem hiding this comment.
Hmm. For some reason they were passing, but I just cleaned and rebuilt everything and now they're failing, so there must have been some operator error.
Until it's fixed on desktop, yes, certain benefits of the changes won't be useful if you're running against desktop's in-box assembly, namely that you won't be able to reuse the content objects nor use a handler on desktop that expects some level of duplex communication. The biggest benefit of the change, though, is that it allows the HttpClient implementation on unix to work with sites that would otherwise cause it to break, and in that sense, it is usable by netstandard libraries: they can be written to target sites that would otherwise be problematic. |
You can't rely on that behavior. You can and probably should just write a .NET Core specific library instead. That way you don't have to pretend like the streaming functionality would actually work on all platforms that claim to support this behavior. Either that or make it throw on other platforms if you were expecting this behavior (or use a different http client library). |
Are we talking about the same thing? The main problem being solved here is that when using HttpClient to connect to some sites, HttpClient fails with exceptions about the request content having been disposed, because the site may start sending the response before the request has been fully received, and HttpClient on unix may then end up disposing of the request content while it's still in use. That's fixed by this, because now the content won't be disposed. Such sites already worked on both desktop and .NET Core on Windows (as the Windows implementation wouldn't process the response until it had fully sent the request), so now you can use HttpClient with such sites regardless of whether it's desktop, core on Windows, or core on Unix. There is a secondary benefit of this change that it then allows for more general streaming behavior if supported by the handler. That is specific to the handler, and thus specific to core. |
HttpClient on Unix fails for some sites, in cases where the site sends a response before it’s fully received the entirety of the request. That’s because HttpClient has logic to Dispose of the request content once the Task returned from the handler’s SendAsync completes, and unlike WinHTTP, libcurl doesn’t require that the whole request be sent before any of the response is processed. That means that CurlHandler may receive the response (or the response headers), complete the SendAsync task, but still be reading from the request content, which may then be disposed of by HttpClient while it’s still in use, causing potential corruption, exceptions, etc. The Dispose behavior has other problems with it. A more generalized version of the above is that it prevents handlers that want to support full duplex communication, since it essentially requires that the whole request be sent before the response is processed. It also forces extra cost onto consuming code in some situations, as it means that the request content objects can’t be pooled and reused (since disposal typically invalidates objects for such a purpose, though in some cases a custom content could be developed to override and ignore Dispose). A variety of approaches were explored to address this: - Change CurlHandler to only complete the SendAsync Task when both the request has been fully sent and the appropriate portion of the response has been received. This is problematic, though, as with the pull model employed by libcurl, we don’t know reliably when the request has been fully sent. - Change the disposal to be handled by the handlers rather than by HttpClient. This could help in that a handler like CurlHandler could simply defer disposal until libcurl tells it the “easy” operation is done. But it has other downsides. Today code can use an HttpClientHandler without HttpClient, e.g. by using HttpMessageInvoker instead of HttpClient, in which case it bypasses this disposal logic (and other logic provided by the relatively thin HttpClient wrapper). If we were to move the disposal logic to the HttpClientHandler, now such code that was never having its request content disposed will start to, which is bad and exactly the opposite of what we want. It also causes problems for chains of handlers, e.g. if HttpClient is given a handler that wraps CurlHandler, and that handler expects to be able to look at the request content after the SendAsync Task from CurlHandler completes but before it itself completes its SendAsync Task. And while such a scenario may seem far fetched, we effectively have it in the bits today: the diagnostic handler that's used to pass details off to DiagnosticListeners is injected in the middle like this, and it does pass off the request content after the wrapped SendAsync Task completes... if a listener was expecting a non-disposed request content at that point, it would be sorely disappointed. - Simply stop disposing. The primary downside here is existing code that may have expected disposal to happen, e.g. if a StreamContent wrapping a FileStream isn't explicitly disposed of by the app code, cleanup of the FileStream will be left to its finalizer. The least bad option is to simply stop disposing. This PR makes that change.
stephentoub
force-pushed
the
stop_disposing_content
branch
from
e4b0f96
to
6d06c39
Compare
|
@dotnet-bot test outerloop netcoreapp Windows_NT Debug |
No but I think it applies regardless. I didn't realize the design of HttpClient itself was incompatible with some handlers (in this case libcurl).
Sorta but not really, without these http client changes, you can't even author a handler to act like this today. |
Hence the "specific to core" part. |
|
Except for the streaming part... regardless, I hope this change makes it back into full framework someday... |
PR #19082 changed .NET Core behavior to no longer dispose request content after send. However, the original behavior remains in .NET Framework. So, the test should be skipped on NETFX. Closes #30987
dotnet/corefx#19082 Add PostJsonAsync
…t/corefx#31031) PR dotnet/corefx#19082 changed .NET Core behavior to no longer dispose request content after send. However, the original behavior remains in .NET Framework. So, the test should be skipped on NETFX. Closes dotnet/corefx#30987 Commit migrated from dotnet/corefx@2a5e017
HttpClient on Unix fails for some sites, in cases where the site sends a response before it’s fully received the entirety of the request. That’s because HttpClient has logic to Dispose of the request content once the Task returned from the handler’s SendAsync completes, and unlike WinHTTP, libcurl doesn’t require that the whole request be sent before any of the response is processed. That means that CurlHandler may receive the response (or the response headers), complete the SendAsync task, but still be reading from the request content, which may then be disposed of by HttpClient while it’s still in use, causing potential corruption, exceptions, etc.
The Dispose behavior has other problems with it. A more generalized version of the above is that it prevents handlers that want to support full duplex communication, since it essentially requires that the whole request be sent before the response is processed. It also forces extra cost onto consuming code in some situations, as it means that the request content objects can’t be pooled and reused (since disposal typically invalidates objects for such a purpose, though in some cases a custom content could be developed to override and ignore Dispose).
A variety of approaches were explored to address this:
The least bad option is to simply stop disposing. This PR makes that change.
Fixes https://github.com/dotnet/corefx/issues/9006
Fixes https://github.com/dotnet/corefx/issues/16259
Fixes https://github.com/dotnet/corefx/issues/1794
cc: @davidsh, @CIPop, @geoffkizer, @Priya91, @davidfowl, @mikeharder