Allow OperationCanceledExceptions to propagate out of ClientWebSocket.ConnectAsync #38204
Conversation
stephentoub
force-pushed
the
wsconnectcancel
branch
from
4e2088d to
e8fcc0e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
stephentoub
force-pushed
the
wsconnectcancel
branch
from
e8fcc0e to
e17444b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
| @@ -223,10 +223,12 @@ public async Task ConnectAsyncCore(Uri uri, CancellationToken cancellationToken, | |||
| Abort(); | |||
| response?.Dispose(); | |||
|
|
|||
| if (exc is WebSocketException) | |||
| if (exc is WebSocketException || | |||
| (exc is OperationCanceledException && cancellationToken.IsCancellationRequested)) | |||
There was a problem hiding this comment.
Is there any risk of getting an OperationCanceledException for a different CancellationToken?
There was a problem hiding this comment.
Is there any risk of getting an OperationCanceledException for a different CancellationToken?
It's certainly possible, e.g. for an Abort() call, which is why we check whether this cancellation token had cancellation requested. If something else caused cancellation, then from the perspective of this call it wasn't the one to cancel it, and so it continues to wrap the exception. If something else caused cancellation but this was also canceled, then it doesn't matter which actually triggered things to cancel, and so we allow it through.
|
The PR looks good to me as well. In the end, I feel like it is even more important to document this kind of non-happy-cases (see linked issue). |
Every bug fix has the potential to be a breaking change for someone. We definitely take such things into account, and in general we're wary of changing the type of exception thrown in certain circumstances. In this case, however, I think it's ok. Across .NET the broad pattern is that cancellation results in OperationCanceledException (or derived exceptions), and it's a bug that this case was wrapping them in another layer. |
stephentoub
force-pushed
the
wsconnectcancel
branch
from
e17444b to
a8bdb22
Compare
|
/azp run corefx-outerloop-windows |
|
/azp run corefx-outerloop-linux |
|
/azp run corefx-outerloop-osx |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 similar comments
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
….ConnectAsync They're currently being wrapped in WebSocketExceptions, but they should be allowed to escape unwrapped.
stephentoub
force-pushed
the
wsconnectcancel
branch
from
a8bdb22 to
c51a433
Compare
|
/azp run corefx-outerloop-windows |
|
Azure Pipelines successfully started running 1 pipeline(s). |
….ConnectAsync (dotnet/corefx#38204) They're currently being wrapped in WebSocketExceptions, but they should be allowed to escape unwrapped. Commit migrated from dotnet/corefx@513f9f9
They're currently being wrapped in WebSocketExceptions, but they should be allowed to escape unwrapped.
Fixes https://github.com/dotnet/corefx/issues/38199
cc: @davidsh, @matthid