Implement Socket.Send/ReceiveAsync cancellation support #34212
Conversation
In .NET Core 2.1 we added Socket.Send/ReceiveAsync overloads that worked with {ReadOnly}Memory, and in doing so also added CancellationToken arguments, but in 2.1 those implementations only did an up-front check for cancellation and then ignored the token for the remainder of the operation.
This PR addresses that, mostly. There are two main ways this could be done, both with their own upsides and downsides:
- Track the individual async operations in flight on a Socket. This provides the most granularity, and does match best with CancellationToken being on the individual Send/ReceiveAsync calls. However, it doesn't actually apply for TCP operations on Windows, where all pending operations are canceled regardless of whether CancelIoEx was targeted at a specific native overlapped or not. And it increases the size of SocketAsyncEventArgs by several fields. And in the scenarios we're aware of, cancellation is generally applied as part of some kind of teardown, where it's unlikely any other operations on the socket will matter after one is canceled. And it requires tendrils into both the Windows and Unix implementations, in order to track things like the NativeOverlapped pointer on Windows. It also would require significant work to plumb through to support operations implemented with the older APM implementations, which we do for the Task-based Send/ReceiveAsync when there's already a pending send/receive using the cached SocketAsyncEventArgs instance.
- Cancel all pending I/O on a socket when any operation on the socket is canceled. This results in a much simpler solution, with very little platform-specific code and that doesn't need any additional fields on SocketAsyncEventArgs, and works regardless of how we've implemented the operation, since it applies to any I/O on the socket and doesn't require us to know the specific NativeOverlapped pointer. It does sacrifice the fine-grained ability to cancel individual UDP operations on Windows, or any individual operation on Linux, without also canceling other pending operations on the same socket.
After many back and forths playing with both implementations, I've opted for the latter. If in the future we decide it's not fine-grained enough, we could do the former.
|
Linux test failures may be related to this change: |
|
They are. I need to change the Unix implementation to not update the operation list when canceling the operations in it. |
| // That way, there's no concern about leaking a registration. | ||
| if (_cancellationState != CancellationState_None) | ||
| { | ||
| var sw = new SpinWait(); |
There was a problem hiding this comment.
would it make sense to call this only when needed? (unless it is really cheap). I would expect that cases when we have contention on state would be rare.
There was a problem hiding this comment.
It's cheap. SpinWait is a struct, so
var sw = new SpinWait();
is identical to:
SpinWait sw = default;
|
Some quick thoughts: Send can be asynchronous too, when the kernel send buffer is full. We should add tests for cancelling pending sends. This makes me wonder, though: You say above that CancelIoEx cancels all outstanding IO, even when you specify a particular overlapped to cancel. Does this mean both sends and receives are cancelled always? I can sort of understand why CancelIoEx on a given Receive would cancel all pending receives -- if it didn't, it could result in weird behavior where receive #2 succeeds but receive #1 was cancelled. But this doesn't apply across send and receive. Somewhat similarly -- if you have multiple pending receives, does it matter which one you cancel? I could imagine that if you have three pending receives and cancel #2, it would result in cancelling #2 and #3, since #3 is behind #2 in the waiting order, but not cancel #1 -- because this can still complete reliably. Of course, I'm just speculating here, but it would be good to know for sure. (If you can link the test code you used here, I'd be happy to play around with it myself) |
|
I'm conflicted about this change because of how it could impact our ability to make structural changes in async handling in the future -- particularly on Linux. (See below for details.) In particular, we seem to be committing to a certain cancellation semantic, where cancelling a single operation cancels all pending operations. A model where cancellation only cancels the specified operation seems like it would be easier to maintain regardless of implementation changes. That said, maybe we would be fine changing the semantic in the future; it's a very corner case to have multiple pending receives or sends. But even so, I think I'd feel better if cancelling a receive didn't also cancel sends. In terms of future structural changes, here's what I mean. There's a whole lot of machinery in SocketAsyncContext to manage the operation queues. This is necessary because we allow you to have multiple outstanding receives or sends on a single socket. Windows supports this basically for free, and so we have always supported it on Windows, and thus we decided we needed the same behavior on Linux. But having multiple pending sends or receives is almost never done in practice, and is never (as far as I know) a good idea. Certainly, it's never a good idea on Linux. We obviously want to optimize for the common case (no multiple pending ops), and we do this in some ways, e.g. caching operation objects in SocketAsyncContext and caching reusable tasks at a higher level. But we still have a lot of code and complexity to deal with the multiple pending ops case. I'd like to see us move to a different approach here -- at least for Linux, possibly for Windows as well. Instead of maintaining our own queues and operations within them, just have a readerLock and a writerLock that (asynchronously) provides mutual exclusion (i.e. queuing) when multiple pending operations occur. This allows us to simplify the core code so that it only has to deal with a single pending read/write at a time, and avoids the need to cache operation objects (we can store state on the context itself), etc. I think this would be a significant improvement to the structure of the code, and possibly enable some additional optimizations in the future that are somewhat painful now. Taking this even further, we could choose to do this at a higher level so that it applies to both Windows and Linux, and allows us to simplify even more code, like the reusable task code. Ideally, I think this would allow a deep unification between the different code paths we have today for SAEA and non-SAEA paths. And -- getting back to the cancellation issue at hand -- it would make it trivial to support per-operation cancel on Windows. Now, the concern I have with this PR is that it introduces a cancellation semantic that is hard to make work in the model I describe above. You'd need extra tracking to know what operations needed to be cancelled. And doing this seems kind of pointless anyway, since what you probably really want is per-operation cancellation. Obviously this is all pretty speculative; I don't know if we will ever get around to making the changes I propose above. But if we're going to do cancellation now, it would be nice to not box ourselves in to behavior that makes future improvements more difficult. Thoughts? |
Having multiple receives can be done if you are implementing a 'scatter-gather' type algorithm trying to use parallel operations. I agree it not a common type of operation but it can be done.
I agree this is problematic and not obvious to developers. |
No, sorry, I should have been more clear. All receives, but sends aren't affected.
No, canceling any cancels them all.
Here you go: using System;
using System.ComponentModel;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
class Program
{
static unsafe void Main()
{
using (var listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
using (var client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
listener.Bind(new IPEndPoint(Dns.GetHostEntry(Dns.GetHostName()).AddressList.Where(a => a.AddressFamily == AddressFamily.InterNetwork).First(), 0));
listener.Listen(1);
client.Connect(listener.LocalEndPoint);
using (Socket server = listener.Accept())
{
// Saturate send buffer
Task<int>[] sendTasks =
(from i in Enumerable.Range(0, 10)
select client.SendAsync(new ArraySegment<byte>(new byte[1000000]), SocketFlags.None)).ToArray();
Console.WriteLine("Validating sends aren't all completing...");
if (Task.WaitAll(sendTasks, 3_000))
{
throw new Exception("Sends all completing while trying to saturate send buffer.");
}
NativeOverlapped* toCancel = null;
var buffer = new WSABuffer { Length = 100, Pointer = Marshal.AllocHGlobal(100) };
Console.WriteLine("Creating additional test sends...");
for (int i = 0; i < 3; i++)
{
int id = i;
NativeOverlapped* overlapped = new Overlapped().Pack((errorCode, numBytes, pOVERLAP) => Console.WriteLine($"Send {id} completed: error code {Marshal.GetLastWin32Error()}"), null);
SocketError error = WSASend(client.Handle, &buffer, 1, out int bytesTransferred, SocketFlags.None, overlapped, IntPtr.Zero);
Console.WriteLine($"Send {id}: {error} {new Win32Exception().Message}");
}
Console.WriteLine("Creating test receives...");
var receives = new NativeOverlapped*[3];
for (int i = 0; i < receives.Length; i++)
{
int id = i;
SocketFlags flags = SocketFlags.None;
receives[i] = new Overlapped().Pack((errorCode, numBytes, pOVERLAP) => Console.WriteLine($"Receive {id} completed: error code {errorCode}"), null);
SocketError error = WSARecv(client.Handle, &buffer, 1, out int bytesTransferred, ref flags, receives[i], IntPtr.Zero);
Console.WriteLine($"Receive {id}: {error} {new Win32Exception().Message}");
}
Console.WriteLine($"Press 0-{receives.Length-1} (or -1 to cancel everything) followed by enter to cancel corresponding receive...");
int input = int.Parse(Console.ReadLine());
bool canceled = input == -1 ?
CancelIoEx(client.Handle, null) :
CancelIoEx(client.Handle, receives[input]);
Console.WriteLine($"CancelIoEx returned {canceled}");
Console.WriteLine("Done. Type Enter to exit.");
Console.ReadLine();
}
}
}
[DllImport("ws2_32", SetLastError = true)]
internal static extern unsafe SocketError WSASend(
IntPtr socketHandle,
WSABuffer* buffers,
int bufferCount,
out int bytesTransferred,
SocketFlags socketFlags,
NativeOverlapped* overlapped,
IntPtr completionRoutine);
[DllImport("ws2_32", SetLastError = true)]
internal static unsafe extern SocketError WSARecv(
IntPtr socketHandle,
WSABuffer* buffer,
int bufferCount,
out int bytesTransferred,
ref SocketFlags socketFlags,
NativeOverlapped* overlapped,
IntPtr completionRoutine);
[StructLayout(LayoutKind.Sequential)]
internal struct WSABuffer
{
internal int Length;
internal IntPtr Pointer;
}
[DllImport("kernel32", SetLastError = true)]
internal static extern unsafe bool CancelIoEx(IntPtr handle, NativeOverlapped* lpOverlapped);
}
Me, too. Hence my "After many back and forths playing with both implementations" comment 😄
Yes, though from a user perspective, we're doing that regardless of which model we implement. From what I've seen, it's generally relatively rare for sends to not complete synchronously or quickly, which suggests cancellation there won't be particularly impactful or noticeable, and on Windows canceling a single TCP receive will cancel all pending receives on that socket, which means that on Windows that will be the observed model regardless of what we do in the .NET layer (which then also means we end up with different behavior across Windows and Unix if we do implement the "cancel just the one" approach, since on Windows you'd still get the "cancel all receives" behavior).
And this actually makes the "cancel just this one" behavior much more invasive today. On Windows, things are structured in a way where we get the Hence, my also being on the fence but landing on this approach. If you think I should go with the "cancel just the one" given all that, though, I certainly can. |
In .NET Core 2.1 we added Socket.Send/ReceiveAsync overloads that worked with {ReadOnly}Memory, and in doing so also added CancellationToken arguments, but in 2.1 those implementations only did an up-front check for cancellation and then ignored the token for the remainder of the operation.
This PR addresses that, mostly. There are two main ways this could be done, both with their own upsides and downsides:
After many back and forths playing with both implementations, I've opted for the latter. If in the future we decide it's not fine-grained enough, we could do the former.
Fixes #24430
cc: @geoffkizer, @davidsh, @wfurt