Finalize Socket API upgrade

[scalablecory]

Not included in API writeup below:

Should we stop adding to SocketTaskExtensions and make all of these APIs (and all the existing APIs) on top of Socket directly?

These extensions have poor discoverability and were initially added for a Task compat library for Framework. With .NET 5, it's a good chance to clean things up.

Gathered send, scattered receive

Previous APIs took an IList<ArraySegment<byte>, new APIs take an IReadOnlyList<Memory<byte>>. They are also cancellable and return ValueTask.

ReadOnlySequence

We may want to have gathered Send overloads for ReadOnlySequence. There is no writable sequence concept yet, so we wouldn't have a nice symmetry, but that's probably OK.

Why IReadOnlyList<ROM> instead of ReadOnlySequence? Multi-segment sequences are non-trivial to construct outside of Pipelines and would likely need some additional classes to make simple outside of that.

SocketAsyncEventArgs

SocketAsyncEventArgs is challenging as what are otherwise overloadable method arguments are instead exposed as properties of a class... and there already exist properties that work with byte[] and IList<ArraySegment>. With a little prototyping I've been unable to find a great way to merge the existing APIs functionality-wise with Memory-based ones.

The ValueTask APIs are efficient, so I think it's easy to simply not make a public API change here. We can leave this for future if the need arises.

Connect(ConnectAlgorithm) APIs

We added a SocketAsyncEventArgs version of this API, but not the other overloads. This adds those.

UdpClient

Spanifies and makes cancellable existing APIs.

Proposed APIs

class Socket
{
	// existing: public int Send(IList<ArraySegment<byte>> buffers);
	// existing: public int Send(IList<ArraySegment<byte>> buffers, SocketFlags socketFlags);
	public int Send(IReadOnlyList<ReadOnlyMemory<byte>> buffers, SocketFlags socketFlags = SocketFlags.None);
	
	// existing: public int Receive(IList<ArraySegment<byte>> buffers);
	// existing: public int Receive(IList<ArraySegment<byte>> buffers, SocketFlags socketFlags);
	public int Receive(IReadOnlyList<Memory<byte>> buffers, SocketFlags socketFlags = SocketFlags.None);
}

class SocketTaskExtensions
{
	// existing: public static Task<int> SendAsync(IList<ArraySegment<byte>> buffers, SocketFlags socketFlags);
	public static ValueTask<int> SendAsync(IReadOnlyList<ReadOnlyMemory<byte>> buffers, SocketFlags socketFlags = SocketFlags.None, CancellationToken cancellationToken = default);
	
	// existing: public static Task<int> ReceiveAsync(this Socket socket, IList<ArraySegment<byte>> buffers, SocketFlags socketFlags);
	public static ValueTask<int> ReceiveAsync(IReadOnlyList<Memory<byte>> buffers, SocketFlags socketFlags = SocketFlags.None, CancellationToken cancellationToken = default);
	
	// existing: public static ValueTask ConnectAsync (this Socket socket, EndPoint remoteEP, CancellationToken cancellationToken);
	public static ValueTask ConnectAsync (this Socket socket, EndPoint remoteEP, ConnectAlgorithm connectAlgorithm, CancellationToken cancellationToken = default);
	
	// existing: public static ValueTask ConnectAsync (this Socket socket, IPAddress[] addresses, int port, CancellationToken cancellationToken);
	public static ValueTask ConnectAsync (this Socket socket, IPAddress[] addresses, int port, ConnectAlgorithm connectAlgorithm, CancellationToken cancellationToken = default);
	
	// existing: public static ValueTask ConnectAsync (this Socket socket, string host, int port, CancellationToken cancellationToken);
	public static ValueTask ConnectAsync (this Socket socket, string host, int port, ConnectAlgorithm connectAlgorithm, CancellationToken cancellationToken = default);
}

class TcpClient
{
	// existing: public ValueTask ConnectAsync (string host, int port, CancellationToken cancellationToken);
	public ValueTask ConnectAsync (string host, int port, ConnectAlgorithm connectAlgorithm, CancellationToken cancellationToken);
	
	// existing: public ValueTask ConnectAsync (IPAddress address, int port, CancellationToken cancellationToken);
	public ValueTask ConnectAsync (IPAddress address, int port, ConnectAlgorithm connectAlgorithm, CancellationToken cancellationToken);
	
	// existing: public ValueTask ConnectAsync (IPAddress[] addresses, int port, CancellationToken cancellationToken);
	public ValueTask ConnectAsync (IPAddress[] addresses, int port, ConnectAlgorithm connectAlgorithm, CancellationToken cancellationToken);
}

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(ReadOnlySpan<byte> datagram, CancellationToken cancellationToken);
	
	// existing: public Task<int> SendAsync(byte[] datagram, int bytes, IPEndPoint endPoint);
	public ValueTask<int> SendAsync(ReadOnlySpan<byte> datagram, IPEndPoint endPoint, CancellationToken cancellationToken);

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

(Edit @geoffkizer May 05 2021: I removed DisconnectAsync from above since it got approved and implemented separately.)

[scalablecory]

Triage:

• General agreement that taking an IList<Memory> is a good choice over ReadOnlySequence. Consider what usage we might open up using IEnumerable instead.
• There is some concern that ConnectAsync() overload count is getting too large. A mock prototype will help prove out how bad Intellisense gets, and we should consider what to do from there -- implementing fewer of the overloads, using an "options" class, or just not implementing them and see how far documentation can help users.
• We can probably leave off the UdpClient.Send taking a host/port, as it seems wildly inefficient and doesn't appear to have broad use.
• DisconnectAsync is play for approval but implementing it is low priority; it's a rarely used API and there is existing tools to build-your-own.

[GSPP]

There are many ways to provide data for sending (e.g. array, memory, sequence, IList<Memory>). This together with the other options increases the overload sprawl. Ways to provide data multiply with ways to set other options such as the IP. Thank god we have default parameters at least.

A way around that multiplication would be to introduce a struct wrapper (a union type) that can wrap any way to provide data:

struct SocketData {
 byte[] Array;
 Memory<byte> Memory;
 IList<Memory<byte>> MemoryList;
 ReadOnlySequence Sequence; //Currently not proposed
 Stream Stream; //Just an idea

 //Implicit conversion operators and constructors here
}

And then a single overload taking such a value:

public void Send(SocketData socketData, ...);

This would be strongly typed to only allow appropriate values. SocketData does not need to be constructed by user code thanks to the implicit conversion. And since SocketData is a struct there would be no allocation.

This concept is so scalable in terms of API surface that it becomes feasible to accept new formats such as a ReadOnlySequence or a Stream.

A similar idea can apply to receiving data. It also applies to ways to specify an endpoint (e.g. single IP, multiple IPs, hostname, eyeballs).

[svick]

@GSPP

SocketData does not need to be constructed by user code thanks to the implicit conversion.

That wouldn't work for the IList<Memory<byte>> case, since you can't have implicit conversions from interfaces.

[Pepsi1x1]

Cancellation support on UdpClient would be wonderful!

[MaxDZ8]

Some feedback.

I've just decided to not use UdpClient and prefer Socket. I've spent a while reading all the various discussions about its functionalities relating especially to cancellation. The provided value is unclear to me. I dream of a day those things will be at least deprecated but with .NET 5 out of the door I guess this will remain my fantasy.

Should we stop adding to SocketTaskExtensions and make all of these APIs (and all the existing APIs) on top of Socket directly?

These extensions have poor discoverability and were initially added for a Task compat library for Framework. With .NET 5, it's a good chance to clean things up.

I agree and support.

The choice I made towards Socket is to prepare for the day those calls will be in Socket. I'm not in the position to evaluate the fall-out but considering the changes in major versions as well as other technologies (such as aspnetcore, for example) I think it would have been very appropriate.

We can probably leave off the UdpClient.Send taking a host/port, as it seems wildly inefficient and doesn't appear to have broad use.

Let me reiterate. I realize my viewport is limited. I think UdpClient must go.

[GF-Huang]

So does UdpClient.Send(ReadOnlySpan<byte>) added into .NET 5?

[svick]

@GF-Huang No, this issue is still open and .Net 5 has been released, which means it won't receive any new APIs.

[geoffkizer]

Re UdpClient changes, the underlying support in Socket is now complete so adding them to UdpClient is unblocked.

I've reopened #864 to get API approval.

[geoffkizer]

I think UdpClient must go.

Yeah, UdpClient does not seem very useful in general. It's usually better to just use Socket directly.

[wfurt]

triage: we should reevaluate what was already done and consider #63162. Updating TcpClient & UdpClient seems like low value. Perhaps we should open and track more specific issues.

[wfurt]

The scatter/gather proposals are tracked by #49941 and #25344.
I don't think we want to invest into TcpClient and UdpClient. The workaround is trivial as both expose the underlying Socket. Closing for now, we should open specific issue if needed to avoid mega issues like this.