Issue #23141 Fix ManagedHandler cancellation Tests #25511
Conversation
WinCPP
changed the title
I know I wrote about that as being an approach to addressing this kind of issue, but I don't believe it's an appropriate approach here; as core library, we should not be leaving async operations still in flight once the Task that's been returned to represent it is marked as canceled. As I'd mentioned previously, the right approach to addressing cancellation of reads/writes in the managed handler is to support cancellation of sends/receives on the underlying socket operations being used, and simply passing the token to those operations, as is already being done. |
|
Yup. I agree. I'm working on finding solution to cancelling read / writes... I hope I have time... |
WinCPP
force-pushed
the
managed_handler_23141
branch
from
a39b11b to
8ff76c2
Compare
WinCPP
changed the title
|
@stephentoub I have re-implemented the code. Appreciate your review comments. Thanks. |
| @@ -308,7 +308,7 @@ private Task WriteFormattedInt32Async(int value, CancellationToken cancellationT | |||
| } | |||
|
|
|||
| // Parse the response status line. | |||
| var response = new HttpResponseMessage() { RequestMessage = request, Content = new HttpConnectionContent(CancellationToken.None) }; | |||
| var response = new HttpResponseMessage() { RequestMessage = request, Content = new HttpConnectionContent(cancellationToken) }; | |||
There was a problem hiding this comment.
Changed to cancellationToken in place of CancellationToken.None because the token passed to ReadNextLineAsync is not further passed down into its call chain and hence there is no way to dispose the underlying streams to trip the operations via an exception.
|
@dotnet-bot retest Linux x64 Release Build please... |
|
@WinCPP, thank you, but again, I don't think this is the right approach. To be very clear, I believe the right approach is primarily to implement https://github.com/dotnet/corefx/issues/24430, and then https://github.com/dotnet/corefx/issues/23141 will effectively come for free. |
|
@stephentoub thank you for your feedback. I understand. I thought the idea was to have it just like WinHTTP* wrappers. hmmm #24430 is not up-for-grabs, unfortunately. Can I try working on it and then on this one? |
|
@WinCPP, thanks, you're welcome to try. It's not an easy issue, though. Windows might not be too hard; it'll likely involve P/Invoking to CancelIoEx. On Unix, it'll likely involve changes in the bowels of SocketAsyncContext. For both cases, we need to be sure we're not adding significant cost, in particular when a non-cancelable token is passed in, only incurring additional cost (and as little as possible) when the provided token can be canceled. |
|
In the meantime, I'm going to close this PR. Thank you for your efforts. |
An attempt to solve #23141. There are two parts to the PR.
Improving the cancellation support in ManagedHandler: this employs handling the cancellation via the CancellationToken while allowing the operation to still run in background (as discussed in this comment). The idea is based on the blog article here :-)
Even with the above 'by-pass' fix, the test 'ReadAsStreamAsync_ReadAsync_Cancel_TaskCanceledQuickly' still hanged while doing
ReadAsyncfrom the stream. In case ofManagedHandlerthe response stream is of typeContentLengthReadStreamwhich needed fix on the lines of cancelable functionality inWinHttpResponseStream(here)I am still trying to find out how to close / dispose the HttpConnection or the stream instead of allowing the operations to run in the background, consequences of which I am not very sure of.
@stephentoub, I have created this PR as we discussed here. Appreciate your inputs.
Thanks,
Mandar