Plumb CancellationToken through Socket.Receive/SendAsync #36516
Conversation
|
GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly test is failing. |
Definitely is. I'll take a look. |
In .NET Core 2.1 we added overloads of Send/ReceiveAsync, and we proactively added CancellationToken arguments to them, but those tokens were only checked at the beginning of the call; if a cancellation request came in after that check, the operation would not be interrupted. This PR plumbs the token through so that a cancellation request at any point in the operation will cancel that operation. On Windows we register to use CancelIoEx to request cancellation of the specific overlapped operation on the specific socket. On Unix we use the existing cancellation infrastructure already in place to support the existing custom queueing scheme. Some caveats: - On Windows, canceling a TCP receive will end up canceling all TCP receives pending on that socket, even when we request cancellation of a specific overlapped operation; this is just how cancellation works at the OS level, and there's little we can do about it. It also shouldn't matter much, as multiple pending receives on the same socket are rare. - If multiple concurrent receives or multiple concurrent sends are issued on the same socket, only the first will actually be cancelable. This is because this implementation only plumbs the token through the SocketAsyncEventArgs-based code paths, not the APM based code paths, and currently when using the Task-based APIs, we use the SocketAsyncEventArgs under the covers for only one receive and one send at a time; other receives made while that SAEA receive is in progress or other sends made while that SAEA send is in progress will fall back to using the APM code paths. This could be addressed in the future in various ways, including a) just using the SAEA code paths for all operations and deleting the APM fallback, or b) plumbing cancellation through APM as well. However, for now, this approach addresses the primary use case and should be sufficient. - This only affects code paths to which the CancellationToken passed to Send/ReceiveAsync could reach. If in the future we add additional overloads taking CancellationToken, we will likely need to plumb it to more places.
stephentoub
force-pushed
the
socketcancellation
branch
from
f1e3e3e
to
d78c103
Compare
|
@wfurt, fixed. I needed a try/catch to handle a race condition between the SafeHandle being disposed and trying to use it for cancellation. This showed up in the HTTP tests because we forcefully close the connection as part of cancellation (we could look at revisiting that after this goes in). |
|
/azp run corefx-outerloop-windows |
|
/azp run corefx-outerloop-linux |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run corefx-outerloop-osx |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sebastienros, before merging this, I'd like to make sure this doesn't regress Kestrel benchmarks. What's the best way for me to do that these days? |
|
@stephentoub the benchmarking still requires some human intervention. For benchmarks @sebastienros is doing with me, I provide him a release build of System.Net.Sockets.dll (baseline version and modified version). |
|
There doesn't appear to be any meaningful impact on plaintext, positive or negative, which is good (the goal here was to support additional functionality without negatively impacting perf). The numbers fluctuate a bit from run to run, but here's an example on Windows: and on Linux: |
| // call TryCancel, so we do this after the op is fully enqueued. | ||
| if (cancellationToken.CanBeCanceled) | ||
| { | ||
| operation.CancellationRegistration = cancellationToken.UnsafeRegister(s => ((TOperation)s).TryCancel(), operation); |
There was a problem hiding this comment.
@stephentoub a canceled operation will remain in the queue until it is removed at the next event or the queue gets stopped.
There was a problem hiding this comment.
Yes. Is that a problem?
There was a problem hiding this comment.
Not functionally. It will be alive for longer and maybe keeping some other things alive.
There was a problem hiding this comment.
It will be alive for longer and maybe keeping some other things alive.
Yes, but then again so are the Socket and the SocketAsyncEventArgs. In comparison this shouldn't keep alive much. And if you're canceling the operation, there's a really high likelihood you're also either tearing everything down or about to do something else that will revisit the queue.
…fx#36516) In .NET Core 2.1 we added overloads of Send/ReceiveAsync, and we proactively added CancellationToken arguments to them, but those tokens were only checked at the beginning of the call; if a cancellation request came in after that check, the operation would not be interrupted. This PR plumbs the token through so that a cancellation request at any point in the operation will cancel that operation. On Windows we register to use CancelIoEx to request cancellation of the specific overlapped operation on the specific socket. On Unix we use the existing cancellation infrastructure already in place to support the existing custom queueing scheme. Some caveats: - On Windows, canceling a TCP receive will end up canceling all TCP receives pending on that socket, even when we request cancellation of a specific overlapped operation; this is just how cancellation works at the OS level, and there's little we can do about it. It also shouldn't matter much, as multiple pending receives on the same socket are rare. - If multiple concurrent receives or multiple concurrent sends are issued on the same socket, only the first will actually be cancelable. This is because this implementation only plumbs the token through the SocketAsyncEventArgs-based code paths, not the APM based code paths, and currently when using the Task-based APIs, we use the SocketAsyncEventArgs under the covers for only one receive and one send at a time; other receives made while that SAEA receive is in progress or other sends made while that SAEA send is in progress will fall back to using the APM code paths. This could be addressed in the future in various ways, including a) just using the SAEA code paths for all operations and deleting the APM fallback, or b) plumbing cancellation through APM as well. However, for now, this approach addresses the primary use case and should be sufficient. - This only affects code paths to which the CancellationToken passed to Send/ReceiveAsync could reach. If in the future we add additional overloads taking CancellationToken, we will likely need to plumb it to more places. Commit migrated from dotnet/corefx@2190a0f
In .NET Core 2.1 we added overloads of Send/ReceiveAsync, and we proactively added CancellationToken arguments to them, but those tokens were only checked at the beginning of the call; if a cancellation request came in after that check, the operation would not be interrupted.
This PR plumbs the token through so that a cancellation request at any point in the operation will cancel that operation. On Windows we register to use CancelIoEx to request cancellation of the specific overlapped operation on the specific socket. On Unix we use the existing cancellation infrastructure already in place to support the existing custom queueing scheme.
Some caveats:
Fixes https://github.com/dotnet/corefx/issues/24430
cc: @geoffkizer, @davidsh, @wfurt, @tmds