Open scalablecory opened 4 years ago
Triage:
IList<Memory>
is a good choice over ReadOnlySequence
. Consider what usage we might open up using IEnumerable
instead.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).
@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.
Cancellation support on UdpClient would be wonderful!
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.
So does UdpClient.Send(ReadOnlySpan<byte>)
added into .NET 5?
@GF-Huang No, this issue is still open and .Net 5 has been released, which means it won't receive any new APIs.
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.
I think UdpClient must go.
Yeah, UdpClient does not seem very useful in general. It's usually better to just use Socket directly.
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.
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 ofSocket
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 anIReadOnlyList<Memory<byte>>
. They are also cancellable and returnValueTask
.ReadOnlySequence
We may want to have gathered
Send
overloads forReadOnlySequence
. There is no writable sequence concept yet, so we wouldn't have a nice symmetry, but that's probably OK.Why
IReadOnlyList<ROM>
instead ofReadOnlySequence
? 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 withbyte[]
andIList<ArraySegment>
. With a little prototyping I've been unable to find a great way to merge the existing APIs functionality-wise withMemory
-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
(Edit @geoffkizer May 05 2021: I removed DisconnectAsync from above since it got approved and implemented separately.)