follow up on duplex operation for HTTP/2#38226
Conversation
|
I pushed some changes to address lingering issues raised through this conversation or related issues: StartWriteAsync can fail as we have seen in #38481 and #35466
In some of the traces, we probably got GOAWAY and failed right at RequestCreditAsync() SendHeadersAsync() To address this I moved disposal of CreditManager objects from CheckForShutdown() to actual Dispose, so they follow life cycle of Http2Connection instead of being possibly disposed by GOAWAY frame. On that note, if somebody Disposes HttpClient -> Http2Connection we are still on same situation. With #34638 tracking remaining work for GOAWAY cases I think this fixes #35466 I added missing await to collect error on responseHeadersTask As I addresses comments from @geoffkizer I'm not aware of any remaining work for #35652 In that part I bumped to #9071 where cancellation basically does not work for HttpContent. For now, I simply extended test SlowTestStream() to do direct cancelation there so I can verify RST scenario. fixes #38391 Cancelation may need some more work but that should be probably separate PR . It would be great if you can take another look @stephentoub and @geoffkizer |
|
After chatting with @stephentoub, I pushed new update to:
|
How does this help? If I understand correctly, you've made it so that we will actually dispose CreditManager and the stream Dictionary earlier. That seems like it makes things worse, not better. |
| { | ||
| await _request.Content.CopyToAsync(writeStream, null, cancellationToken).ConfigureAwait(false); | ||
| // TODO: until #9071 is fixed, cancellation on content.CopyToAsync does not work. | ||
| // To work around it, register delegate and set _abortException as needed. |
There was a problem hiding this comment.
I'm confused here. #9071 is about exposing the cancellationToken publicly. But it looks like there is an internal method that does support cancellation. So why do we need to do this registration thing here?
And what is the registration actually accomplishing? It looks like we don't actually cancel the copy, we just set the exception. So what's the point here?
There was a problem hiding this comment.
CopyToAsync() does take CancelationToken. But than it calls TryGetBuffer() and that fails on steaming content so we fall to
SerializeToStreamAsync(stream, context, cancellationToken);and that is
internal virtual Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken) => SerializeToStreamAsync(stream, context);so as far as I can tell we do loose provided cancelation token.
There may be other ways to do this but there is already code check and throw if the _abortException is set. So the registration simply sets it and we will throw it later when processing the stream.
There was a problem hiding this comment.
Can't we achieve the same thing by just checking the cancellationToken after the CopyToAsync is complete? I.e., no need to actually register here.
There was a problem hiding this comment.
We could but that means we need to finish sending request body, right? For gRPC duplex streaming, that effectively means we cannot cancel request, right?
There was a problem hiding this comment.
How does this particular registration help? When cancellation is requested it'll end up setting _abortException to an OperationCanceledException... but what's paying attention to _abortException? Is something in the CopyToAsync operation monitoring _abortException somehow?
There was a problem hiding this comment.
yes. That was part of your #37223 @stephentoub . We will pick it up with HttpStream in read/write. If there is better way how to propagate cancelationToken itself down, let me know.
There was a problem hiding this comment.
We will pick it up with HttpStream in read/write
But only between calls, right? Meaning, if cancellation is requested we'll end up noticing it after a read or after a write, but if we're in the middle of the read or write, we won't actually notice it until that operation completes, correct?
Without a public overload, I don't know of a better way to achieve it, though. The cancellation token passed into CopyToAsync below will help for some of our own content types, but most won't be given it, and thus this token won't be passed into the the constituent read and write operations. We can address that with HTTP/1.1 by tearing down the connection on cancellation, but we can't do that here.
There was a problem hiding this comment.
Yes, that is correct. As you mentioned, I did not find better easy way. So I take it as opportunistic improvement.
Having that said, adding overload with cancelation should not be controversial so I'm wondering if there is still chance to push it to 3.0. I guess that would be primary choicer if this was on our radar, right?
There was a problem hiding this comment.
adding overload with cancelation should not be controversial so I'm wondering if there is still chance to push it to 3.0. I guess that would be primary choicer if this was on our radar, right?
@terrajobst, @danmosemsft, could we still get something in here?
There was a problem hiding this comment.
Okay, I understand the issue with SerializeToStreamAsync not actually being cancellable. Thanks for clarifying.
But I still don't see why we need to register here. Why not just do cancellationToken.ThrowIfCancellationRequested (or whatever) after CopyToAsync completes?
Registration is only useful if we're actually going to cancel an in-process operation, which is not the case here.
| if (wait) | ||
| { | ||
| await GetWaiterTask().ConfigureAwait(false); | ||
| using (cancellationToken.Register(s => { |
There was a problem hiding this comment.
This cancellation logic seems like it should apply everywhere that GetWaiterTask is called, e.g. ReadDataAsync.
There was a problem hiding this comment.
In other words, GetWaiterTask should take a CancellationToken, and throw if the CancellationToken is cancelled before the waiter is signalled.
There's some logic to do this with a regular TaskCompletionSource here:
However, we're not using a regular TaskCompletionSource here. We're using the resettable source. I'm not quite sure the best way to make this work with cancellation.
@stephentoub thoughts?
There was a problem hiding this comment.
If we need this logic in other places, I agree with @geoffkizer it'd be good to centralize all of this into GetWaiterTask. The downside to that is it'll make GetWaiterTask more expensive. But maybe that doesn't matter.
There was a problem hiding this comment.
@stephentoub Any suggestions re how to make GetWaiterTask cancellable? I'm unclear about the interactions with the resettable task stuff here.
If we need this logic in other places, I agree with @geoffkizer it'd be good to centralize all of this into GetWaiterTask
We do -- see ReadDataAsync for example, we're ignoring the cancellationToken there currently.
The downside to that is it'll make GetWaiterTask more expensive. But maybe that doesn't matter.
Only when a cancellationToken is specified, right? (I realize that's the default behavior with HttpClient, but at least it's possible to avoid this if it really matters)
There was a problem hiding this comment.
When I talk to @stephentoub yesterday, it seems like we always pass cancellationToken token in even if user does not. (To implement timeout and cancelation of all pending requests)
I may need to update the code little bit as I chat with @stephentoub afterwards and we want existing streams working even after Http2Connection is disposed. (which I did not plan for) The main reason to move this @geoffkizer was to not trigger it from processing GOAWAY as we may hit race condition. I'm wondering if we can do same as you did for _writerLock and simply let it go when last reference is gone instead of disposing them explicitly. If we want streams to be process even after Http2Connection is disposed, can we alternatively keep CreditManager same way and keep it (partially?) functional after disposed? I can add some more test around disposing scenarios. |
There's an issue for figuring out how to dispose the _writerLock properly, right? Regardless, I agree that these should be disposed at the same time the _writerLock is disposed. Since we're not doing this currently, I think it's fine to not dispose these at all for now and fix them up as part of the _writerLock fix. We should make a note in the _writerLock issue to fix these as well, and update the related comment. |
|
I made some updates and added more tests @geoffkizer .
I think problematic case was when we pick connection from pool and we receive GOAWAY or call Abort/AbortStreams before we can create new stream. In that case _disposed is set to true and CreditManager will fail. AddStream() already had logic to deal with race and throw exception with |
| _connectionWindow.Dispose(); | ||
| _concurrentStreams.Dispose(); | ||
|
|
||
| // ISSUE #35466 |
There was a problem hiding this comment.
Why was this comment removed? We haven't actually fixed the issue here, have we?
There was a problem hiding this comment.
I think the flow is understood and remaining work tracked by other existing issues. If you have specific 3.0 task let me know or we can keep it open for future investigation.
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
Windows.Debug failed in System.Security.Cryptography.Pkcs |
* follow up on duplex operation * updates * add test for cancelation * log responseHeadersTask if bodyTask fails first * improve cancelation * feedback from review * update SendAsync_ConcurentSendReceive_Fail test * feedback from review * update SendSettingsAckAsync * more HTTP/2 cancelation tests * update test to accept any derivative from OperationCanceledException * feedback from review * roll-back changes in AcceptConnectionAsync
* follow up on duplex operation * updates * add test for cancelation * log responseHeadersTask if bodyTask fails first * improve cancelation * feedback from review * update SendAsync_ConcurentSendReceive_Fail test * feedback from review * update SendSettingsAckAsync * more HTTP/2 cancelation tests * update test to accept any derivative from OperationCanceledException * feedback from review * roll-back changes in AcceptConnectionAsync Commit migrated from dotnet/corefx@cad5159
5 participants
This is follow-up on #36884 to address concerns about UnobservedTaskExceptions and error propagation.
This change wraps all ignored tasks to make sure it does not end up in failed state without seeing the exception.
I also added test case for case when exception happens while sending RequestBody after Response headers are received. When we hit plain IO exception, reading response will fail as it always did so I did not worry about is new test case.
fixes #36855