-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Implement HttpContent and HttpClient cancellation APIs #686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Implements APIs approved in dotnet/corefx#32615 and dotnet#576
We prefer to not open PRs for new APIs until the the APIs have been approved. Can you make the relevant APIs internal until that issue is reviewed/approved? That way the ones that have been approved can proceed in this PR. |
I was advised that I should mark the PR as blocked in the case it's only adding CTs to existing overloads.
They are up for review tomorrow, I assume the PR review will take longer than that, otherwise I can make them internal as you suggested. |
Thanks. By whom? |
|
By @karelz during the last sync meeting. That was only referring to the need of protected virtual Task<Stream> CreateContentReadStreamAsync(CancellationToken cancellationToken);as described in https://github.com/dotnet/corefx/issues/32615#issuecomment-562083237. Cory asked me to add the |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5f21f6f to
6ab3cd2
Compare
src/libraries/Common/src/System/Net/Http/NoWriteNoSeekStreamContent.cs
Outdated
Show resolved
Hide resolved
6ab3cd2 to
0f9bc84
Compare
src/libraries/Common/src/System/Net/Http/NoWriteNoSeekStreamContent.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
Show resolved
Hide resolved
| requestStream, state.TransportContext | ||
| #endif | ||
| ).ConfigureAwait(false); | ||
| await state.RequestMessage.Content.CopyToAsync(requestStream, state.TransportContext).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wanted to we could add a configuration to the System.Net.Http.WinHttpHandler package that would contain a build for .NET 5, and then we could utilize the new APIs in that build. But maybe it's not worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would only help in scenarios where someone wanted to cancel an operation that uploaded request body content (i.e. POST).
I think we can wait for customer feedback and then decide if it is worth adding that ifdef'd code and additional .NET 5 build for the Nuget package.
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Closes #916 (dotnet/corefx#32615), closes #576 and closes #17492 (dotnet/corefx#9071).
This PR implements the following APIs: