Cancellable SocketAsyncEventArgs methods

[scalablecory]

Background and Motivation

We already have (or will likely have, for UDP) a cancellable implementation of these methods, but they are not public. I propose making them public.

There are some scenarios where using SocketAsyncEventArgs will be more performant than the Task-based methods, but you must currently trade off cancellation support or get at the private methods via reflection. I'd like to not depend on reflection anymore.

Proposed API

class Socket
{
    // existing.
    public bool SendAsync(SocketAsyncEventArgs e);
    public bool ReceiveAsync(SocketAsyncEventArgs e);
    public bool SendToAsync(SocketAsyncEventArgs e);
    public bool SendPacketsAsync(SocketAsyncEventArgs e);
    public bool ReceiveFromAsync(SocketAsyncEventArgs e);
    public bool ReceiveMessageFromAsync(SocketAsyncEventArgs e);

    // new.
    public bool SendAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool ReceiveAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool SendToAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool SendPacketsAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool ReceiveFromAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool ReceiveMessageFromAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
}

CC @antonfirsov @geoffkizer

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

We already have (or will likely have, for UDP) a cancellable implementation of these methods, but they are not public. I propose making them public.

There are some scenarios where using SocketAsyncEventArgs will be more performant than the Task-based methods, but you must currently trade off cancellation support or get at the private methods via reflection. I'd like to not depend on reflection anymore.

Proposed API

class Socket
{
    // existing.
    public bool SendAsync(SocketAsyncEventArgs e);
    public bool ReceiveAsync(SocketAsyncEventArgs e);
    public bool SendToAsync(SocketAsyncEventArgs e);
    public bool SendPacketsAsync(SocketAsyncEventArgs e);
    public bool ReceiveFromAsync(SocketAsyncEventArgs e);
    public bool ReceiveMessageFromAsync(SocketAsyncEventArgs e);

    // new.
    public bool SendAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool ReceiveAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool SendToAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool SendPacketsAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool ReceiveFromAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
    public bool ReceiveMessageFromAsync(SocketAsyncEventArgs e, CancellationToken cancellationToken);
}

CC @antonfirsov @geoffkizer

Author:	scalablecory
Assignees:	-
Labels:	api-suggestion, area-System.Net.Sockets
Milestone:	6.0.0

[antonfirsov]

This is a weird API that deviates from typical async patterns, has a very strong hack-taste, and is unprecedented in the BCL. I'm not sure if exposing stuff like this is a good tradeoff.

Can't we instead introduce optimized ValueTask overloads for gathered reads/writes?

[scalablecory]

This is a weird API that deviates from typical async patterns, has a very strong hack-taste, and is unprecedented in the BCL. I'm not sure if exposing stuff like this is a good tradeoff.

I disagree re: API being unprecedented-- CancellationToken is the standard model of cancellation; if anything it makes them less weird to add support.

Are you proposing that SocketAsyncEventArgs is dead and should be kept in maintenance mode going forward? I'd be glad to see that, but we should open an issue to track migrating the features it exposes to the ValueTask overloads, and testing that those have equivalent perf.

[antonfirsov]

re: API being unprecedented -- CancellationToken is the standard model of cancellation

I'm talking about specific API models. CancellationToken is the standard, preferred way -- on Task-based methods. In your proposal you are mixing the EAP model with the Task-based model. I'm not aware of existing similar API-s in the BCL.

Are you proposing that SocketAsyncEventArgs is dead and should be kept in maintenance mode going forward?

No, but it has different patterns for cancellation (eg. CancelConnectAsync(e) which conforms to this guideline). We should either follow that pattern and add Socket.CancelReadAsync(e) + CancelWriteAsync(e), or add new ValueTask-based overloads, but do not mix the two, even though it looks cheap to implement.

My understanding is that we are committed to prefer Task-based stuff over EAP, when it comes to adding new API-s. If I'm not missing anything, the following addition should be sufficient to support the performance requirements of the "Flexible HTTP" stuff:

public class Socket
{
    public ValueTask SendAsync(IReadOnlyList<ReadOnlyMemory<byte>> buffers, CancellationToken cancellationToken = default);
    public ValueTask<int> ReceiveAsync(IReadOnlyList<Memory<byte>> buffers, CancellationToken cancellationToken= default);
}

[geoffkizer]

Are you proposing that SocketAsyncEventArgs is dead and should be kept in maintenance mode going forward? I'd be glad to see that, but we should open an issue to track migrating the features it exposes to the ValueTask overloads, and testing that those have equivalent perf.

"Dead" is probably a little too strong.

I think the Task based APIs are our primary and preferred API, and our long-term goal should be to ensure that users never need to use SAEA instead of Task APIs due to missing features or perf. And yeah, that means we should (a) track issues re missing functionality in the Task APIs vs SAEA. and (b) do perf evaluation to ensure there's no perf gap here.

The main gap I'm aware of today is scatter/gather support, but that's sort of tracked via the issue of whether/how to do scatter/gather in Stream and derived classes.

[antonfirsov]

The main gap I'm aware of today is scatter/gather support, but that's sort of tracked via the issue of whether/how to do scatter/gather in Stream and derived classes.

I assume you mean #25344. That does not include the missing async socket API-s (two methods from #48477 (comment)), and AFAIK there is no tracking issue for those.

[geoffkizer]

I assume you mean #25344.

Yeah, that's what I meant.

[karelz]

Triage:

• Ideally move from SAEA to Task APIs long-term
• We need to commit to not just Sockets, but also on Stream (not implement, but commit)
• OK with perf concerns, but would be better to have real test comparing ValueTask and SAEA
• We need API proposal

[karelz]

Idea: Create replacement issue to better capture the final intent.

[dhhoang]

Hi, is this issue still active?
I'd love to have Socket.CancelReadAsync(e)/CancelWriteAsync(e) as I think it will be inline with the design of SocketAsyncEventArgs. In which case the SocketError code would be set to a specific code e.g. Interrupted.
My use case for this is to time-out individual socket calls. Currently that can be done with the socket.SendAsync(data, CancellationToken) overload but it means there is a cancellation registration for every socket call which would likely reduce performance.

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs

Line 195 in 1e3544e

_registrationToCancelPendingIO = cancellationToken.UnsafeRegister(static s =>

[antonfirsov]

The overloads proposed here would not get rid of that CT registration, since we would be still passing the token. AFAIK most if not all CT based IO operations in .NET create a registration internally, but there several optimizations under the hood to make it as cheap as possible. Eg. in socket PAL in the cases where the socket operation completes synchronously, ProcessIOCPResult won't be invoked and no registration happens.

there is a cancellation registration for every socket call which would likely reduce performance

Is it an assumption, or do you have data proving this?

[dhhoang]

The overloads proposed here would not get rid of that CT registration, since we would be still passing the token. AFAIK most if not all CT based IO operations in .NET create a registration internally, but there several optimizations under the hood to make it as cheap as possible. Eg. in socket PAL in the cases where the socket operation completes synchronously, ProcessIOCPResult won't be invoked and no registration happens.

I am not proposing overloads with CancellationToken as I don't think it makes sense to have them in SocketAsyncEventArgs. I think the SocketAsyncEventArgs methods are supposed to give us lower-level control on socket operations e.g. getting a SocketError . What I am proposing are Socket.CancelXAsync(e) (X might be send/recv/ etc.).
The underlying implementation would be to cancel the operation at the native level, for example, calling CancelIoEx on Windows, and on Linux I think it's about ignoring the epoll event. Essentially it'd do the same action that's invoked when CancellationToken is triggered.

Is it an assumption, or do you have data proving this?

I don't have any data, and I'd be happy to create a benchmark to test this. My thinking is that SendAsync(CancellationToken) also uses SAEA, plus the CT registration, it would likely not be as efficient as using SAEA alone. For our use case we also expect a lot of pending read()/write().

[antonfirsov]

The standard way to achieve this would be to Close/Dispose the socket, which would cancel all pending IO. Of course this would terminate the TCP connection, but for most use-cases we've seen this is acceptable or even desirable. Do you still need to be able receive from the socket after cancelling sends or vice-versa? Note that usually it's not safe to eg. issue a new read after a cancelled one from the application POV, since the status of the cancelled operation is undefined (buffer sent/partially sent/not sent).

[dhhoang]

I've managed to create a benchmark. This is just a basic test, a simplified echo server: https://github.com/dhhoang/dotnet-socket-cancellation
Please have a look at the readme for some results. I think it shows CancellationToken methods are noticeably more expensive than SAEA.

Do you still need to be able receive from the socket after cancelling sends or vice-versa

Yes, that is indeed our use case :)