UdpClient - add Span support

[billknye]

[EDIT] Note: Scoped to UdpClient only - see https://github.com/dotnet/corefx/issues/28131#issuecomment-529619141

Checking out the new Span based socket apis in 2.1 preview 1, I noticed the api of UdpClient doesn't have span based overloads.

Falling back to Socket, it appears there too the SendTo api is missing any overloads for Span and friends.

Digging deeper into SocketPal.Windows, there's no SendTo supporting span there but SocketPal.Unix does seem to have some Span support.

The corresponding receive methods are also lacking Span support.

To add actual non-allocating support, it would also appear that endpoint serialization in these methods would need to be tweaked but that seems to be somewhat the point of SocketAddress so maybe calling code would need to pass the SocketAddress instead of the IPEndPoint/EndPoint?

Was this an intentional gap in the api or just something that wasn't priority / low use case?

Edited by @geoffkizer: Current API proposal:

class UdpClient
{
	// existing: public int Send(byte[] dgram, int bytes);
	public int Send(ReadOnlySpan<byte> datagram);
	
	// existing: public int Send(byte[] dgram, int bytes, IPEndPoint endPoint);
	public int Send(ReadOnlySpan<byte> datagram, IPEndPoint endPoint);
	
	// existing: public int Send(byte[] dgram, int bytes, string hostname, int port);
	public int Send(ReadOnlySpan<byte> datagram, string hostname, int port);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, CancellationToken cancellationToken);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes, IPEndPoint endPoint);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, IPEndPoint endPoint, CancellationToken cancellationToken);

	// existing: public Task<UdpReceiveResult> ReceiveAsync();
	public ValueTask<UdpReceiveResult> ReceiveAsync(CancellationToken cancellationToken);
}

[stephentoub]

just something that wasn't priority / low use case?

Exactly. We've added hundreds of span/memory-based methods based on an analysis of what's most common / impactful, but we fully expect more needs to be uncovered and more APIs will be added in the future. If this particular case is valuable to you, please feel free to submit an API proposal.

[karelz]

Socket.SendTo is covered in https://github.com/dotnet/corefx/issues/35861

Triage: We should give UdpCient some Span overloads. Let's use this issue to track such investigation & work.

[scalablecory]

triage: look at other networking apis and add a mega-issue to spanify them.

[scalablecory]

Closing this to consolidate with mega-issue #33417 to streamline API review. See the proposed API there.

Thanks for the issue.

[geoffkizer]

Reopening to use this issue for API approval.

[geoffkizer]

Note, the underlying Socket APIs (SendTo, ReceiveFrom) have had Span support added, which unblocks implementing these APIs.

[bartonjs]

Video

• The CancellationToken parameters should have defaults, particularly in the ones where byte[] becomes ReadOnlyMemory<byte>
• There was some debate as to the mismatch between current Task-returning members and new ValueTask-returning members, and "all the new ones here are ValueTask" was determined to be acceptable.

namespace System.Net.Sockets
{
    partial class UdpClient
    {
	// existing: public int Send(byte[] dgram, int bytes);
	public int Send(ReadOnlySpan<byte> datagram);
	
	// existing: public int Send(byte[] dgram, int bytes, IPEndPoint endPoint);
	public int Send(ReadOnlySpan<byte> datagram, IPEndPoint endPoint);
	
	// existing: public int Send(byte[] dgram, int bytes, string hostname, int port);
	public int Send(ReadOnlySpan<byte> datagram, string hostname, int port);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, CancellationToken cancellationToken = default);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes, IPEndPoint endPoint);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, IPEndPoint endPoint, CancellationToken cancellationToken = default);

	// existing: public Task<UdpReceiveResult> ReceiveAsync();
	public ValueTask<UdpReceiveResult> ReceiveAsync(CancellationToken cancellationToken);
    }
}

[lateapexearlyspeed]

Hi, I would like to try this and already working on it now.

[geoffkizer]

I think we need to revisit this, unfortunately. The proposal above is ignoring some issues around nullability.

Here's the actual current API (sync only for simplicity; async has the same issues)

    partial class UdpClient
    {
	public int Send(byte[] dgram, int bytes);
	public int Send(byte[] dgram, int bytes, IPEndPoint? endPoint);
	public int Send(byte[] dgram, int bytes, string? hostname, int port);
    }

Notice the nullability on IPEndPoint and hostname. If we want to be strictly consistent with the existing APIs, then the Span/Memory-based APIs should have similar nullability.

However, having these arguments be nullable doesn't really make any sense. Using null for either endPoint or hostname results in the same behavior as just calling the overloads without endPoint/hostname. So it's weird that these APIs ever supported null values.

Note this isn't an issue of incorrect nullability annotations -- the existing APIs are explicitly implemented to handle nulls here. This itself could be considered a problem, but that's how it is.

So it seems like we have three options here, from most conservative to least:

(1) New Span/Memory APIs match existing API nullability and semantics when null is used
(2) New Span/Memory APIs don't accept nulls. So they are different than the old APIs, but more sensible.
(3) New Span/Memory APIs don't accept nulls, and we change nullability annotations on the existing APIs as well. We would leave the existing null handling for them, in case anyone depends on it, but the new and APIs would look consistent and sensible.

Thoughts?

Edit: On reflection, I think we should just do (1); anything else here is a change that's not worth the trouble.

[geoffkizer]

Per email, this is approved by API review.

Updated APIs:

namespace System.Net.Sockets
{
    partial class UdpClient
    {
	// existing: public int Send(byte[] dgram, int bytes);
	public int Send(ReadOnlySpan<byte> datagram);
	
	// existing: public int Send(byte[] dgram, int bytes, IPEndPoint? endPoint);
	public int Send(ReadOnlySpan<byte> datagram, IPEndPoint? endPoint);
	
	// existing: public int Send(byte[] dgram, int bytes, string? hostname, int port);
	public int Send(ReadOnlySpan<byte> datagram, string? hostname, int port);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, CancellationToken cancellationToken = default);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes, IPEndPoint? endPoint);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, IPEndPoint? endPoint, CancellationToken cancellationToken = default);

	// existing: public Task<int> SendAsync(byte[] datagram, int bytes, string? hostname, int port);
	public ValueTask<int> SendAsync(ReadOnlyMemory<byte> datagram, string? hostname, int port, CancellationToken cancellationToken = default);

	// existing: public Task<UdpReceiveResult> ReceiveAsync();
	public ValueTask<UdpReceiveResult> ReceiveAsync(CancellationToken cancellationToken);
    }
}