Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Socket API changes mega-issue #33418

Closed
scalablecory opened this issue Mar 10, 2020 · 16 comments
Closed

Socket API changes mega-issue #33418

scalablecory opened this issue Mar 10, 2020 · 16 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Milestone

Comments

@scalablecory
Copy link
Contributor

scalablecory commented Mar 10, 2020

This collects all the Socket APIs that have been approved: #861 #921 #938. (Edit: Also #43933.)

(Edit: removed the async overloads that elide SocketFlags, since this isn't a complete set. See #43934 instead.)

class Socket
{
	// existing: public static bool ConnectAsync(SocketType socketType, ProtocolType protocolType, SocketAsyncEventArgs e);
	public static bool ConnectAsync (SocketType socketType, ProtocolType protocolType, SocketAsyncEventArgs e, ConnectAlgorithm connectAlgorithm);
}

// new enum
enum ConnectAlgorithm
{
    // use existing behavior.
    Default,

    // use a Happy Eyeballs-like algorithm to connect.
    Parallel = 1
}

These APIs were approved and implemented in #40750:

class SocketTaskExtensions
{
	// existing: public static Task ConnectAsync (this Socket socket, EndPoint remoteEP);
	public static ValueTask ConnectAsync (this Socket socket, EndPoint remoteEP, CancellationToken cancellationToken);
	
	// existing: public static Task ConnectAsync (this Socket socket, IPAddress address, int port);
	public static ValueTask ConnectAsync (this Socket socket, IPAddress address, int port, CancellationToken cancellationToken);
	
	// existing: public static Task ConnectAsync (this Socket socket, IPAddress[] addresses, int port);
	public static ValueTask ConnectAsync (this Socket socket, IPAddress[] addresses, int port, CancellationToken cancellationToken);
	
	// existing: public static Task ConnectAsync (this Socket socket, string host, int port);
	public static ValueTask ConnectAsync (this Socket socket, string host, int port, CancellationToken cancellationToken);
}	

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

These were implemented in #47229:

class Socket
{
	// existing: public static Task<int> SendToAsync(this Socket socket, ArraySegment<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP);
	public ValueTask<int> SendToAsync(ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);

	// existing: public static Task<SocketReceiveFromResult> ReceiveFromAsync(this Socket socket, ArraySegment<byte> buffer, SocketFlags socketFlags, EndPoint remoteEndPoint);
	public ValueTask<SocketReceiveFromResult> ReceiveFromAsync(Memory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);

	// existing: public static Task<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(this Socket socket, ArraySegment<byte> buffer, SocketFlags socketFlags, EndPoint remoteEndPoint);
	public ValueTask<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(Memory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);

}

This was implemented in #46285:

class Socket
{
        // existing: public int ReceiveMessageFrom(byte[] buffer, int offset, int size, ref System.Net.Sockets.SocketFlags socketFlags, ref System.Net.EndPoint remoteEP, out System.Net.Sockets.IPPacketInformation ipPacketInformation);
        public int ReceiveMessageFrom(Span<byte> buffer, ref System.Net.Sockets.SocketFlags socketFlags, ref System.Net.EndPoint remoteEP, out System.Net.Sockets.IPPacketInformation ipPacketInformation);
}
class Socket
{
	// existing: public int SendTo(byte[] buffer, EndPoint remoteEP);
	public int SendTo(ReadOnlySpan<byte> buffer, EndPoint remoteEP);

	// existing: public int SendTo(byte[] buffer, SocketFlags socketFlags, EndPoint remoteEP);
	public int SendTo(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP);

	// existing: public int ReceiveFrom(byte[] buffer, ref EndPoint remoteEP);
	public int ReceiveFrom(Span<byte> buffer, ref EndPoint remoteEP);

	// existing: public int ReceiveFrom(byte[] buffer, int offset, int size, SocketFlags socketFlags, ref EndPoint remoteEP);
	public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, ref EndPoint remoteEP);
}

These were implemented in #53340:

class Socket
{
	// existing: public static Task<Socket> AcceptAsync (this Socket socket);
	public ValueTask<Socket> AcceptAsync (CancellationToken cancellationToken);
	
	// existing: public static Task<Socket> AcceptAsync (this Socket socket, Socket acceptSocket);
	public ValueTask<Socket> AcceptAsync (Socket acceptSocket, CancellationToken cancellationToken);
}

class TcpListener
{
	// existing: public Task<Socket> AcceptSocketAsync ();
	public ValueTask<Socket> AcceptSocketAsync (CancellationToken cancellationToken);
	
	// existing: public Task<TcpClient> AcceptTcpClientAsync ();
	public ValueTask<TcpClient> AcceptTcpClientAsync (CancellationToken cancellationToken);
}
@scalablecory scalablecory added the api-approved API was approved in API review, it can be implemented label Mar 10, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Sockets untriaged New issue has not been triaged by the area owner labels Mar 10, 2020
@scalablecory
Copy link
Contributor Author

Yet to be approved: #33417

@andre-ss6
Copy link

Is this ready for implementation? Or is there some kind of obstacle? If not, what would be the level of complexity for a implementation here?

@geoffkizer
Copy link
Contributor

#40750 implemented the ConnectAsync overloads (not including Happy Eyeballs support).

@geoffkizer
Copy link
Contributor

I'm wondering about the overloads of SendToAsync, ReceiveFromAsync, and ReceiveMessageFromAsync that omit the SocketFlags argument.

It looks to me like we don't actually have overloads of SendAsync/ReceiveAsync that omit the SocketFlags argument. So I'm wondering why we are defining overloads for these other methods. Seems like we should define them for all, or none.

@scalablecory Am I missing something here?

@scalablecory
Copy link
Contributor Author

@geoffkizer I believe suggestion came from @wfurt that most of the time you have no flags, but it has been some months.

@geoffkizer
Copy link
Contributor

most of the time you have no flags,

I agree with that. But if we're adding overloads without flags, we should add overloads for Send and Receive as well.

@geoffkizer
Copy link
Contributor

Note, per approval of #43901, we should add the new async overloads to Socket itself, not SocketTaskExtensions. I will update above.

@antonfirsov
Copy link
Member

Edit: removed the async overloads that elide SocketFlags, since this isn't a complete set. See #43934 instead.

@geoffkizer the purpose of this issue is to collect already approved API-s. Therefore (if I'm not getting it wrong) you removed approved variants, which are also missing from #43934 since that one only contains unapproved ones.

@geoffkizer
Copy link
Contributor

I did remove approved variants without SocketFlags args, because I didn't want them to get added in isolation until we have a full story with #43934.

I thought the overloads I removed from here were tracked on #43934 already. It looks like they are there now; did you add them?

antonfirsov added a commit that referenced this issue Nov 16, 2020
…44591)

Some `SendReceive` socket tests may be prone to timing issues on CI. This seems to be the root cause of #1712. We need a reliable way to run such tests to unblock the work on new UDP socket API-s in #33418.

This PR defines a new `SendReceiveNonParallel` test group, moving `SendToRecvFrom_Datagram_UDP` into that group. Since this is already a significant reorganization, it seemed reasonable to also:
- Harmonize naming: all SendReceive test classses are now named either  `SendReceive_[SubVariant]` 
 or `SendReceiveNonParallel_[SubVariant]`
- Split `SendReceive.cs` into multiple files:
    - `SendReceive.cs` for the parallel variants
    - `SendReceiveNonParallel.cs` for the new, non-parallel variants
    - Rename the non-generic class `SendReceive` to `SendReceiveMisc` (to avoid name collision and confusion with `SendReceive<T>`) and move it to `SendReceiveMisc.cs` 
    - Move `SendReceiveListener` and `SendReceiveUdpClient` to separate files, rename `SendReceiveListener` to `SendReceiveTcpClient`
@PJB3005
Copy link
Contributor

PJB3005 commented Apr 23, 2021

I am interested in taking up the datagram-related APIs (basically #938's contents) if that's possible.

@antonfirsov
Copy link
Member

@PJB3005 sure, help is always welcome!

Note that he async versions have been added in #47229, but the sync Span methods are still unimplemented. #46285 is an example of adding such a sync overload. #46600 is also an interesting problem around that space. Testing can get a bit tricky, but I'm happy to help with that. Feel free to ask any question!

PJB3005 added a commit to PJB3005/runtime that referenced this issue Apr 27, 2021
antonfirsov pushed a commit that referenced this issue May 10, 2021
* Adds synchronous span APIs for datagram sockets.

Part of #33418/#938

* Doc comments for new APIs.

* Fix review nits.

* Add 0-byte send tests.
@antonfirsov
Copy link
Member

@geoffkizer @scalablecory is there a reason we did not propose Task-based ConnectAlgorithm overloads?

@scalablecory
Copy link
Contributor Author

@geoffkizer @scalablecory is there a reason we did not propose Task-based ConnectAlgorithm overloads?

good question, seems like an oversight or possibly the issues were not merged fully.

@deeprobin
Copy link
Contributor

deeprobin commented Oct 21, 2022

Would it be possible for us to split the proposal into several issues?

It is easy to lose track of what is already implemented and what is not.

@wfurt
Copy link
Member

wfurt commented Dec 5, 2022

triage: it seems mostly done. We should check and open new issue for the remaining work.

@liveans
Copy link
Member

liveans commented Jun 22, 2023

Triage: Almost all of the work has been done on this issue.
For the remaining work (Happy Eyeball Algorithm) created another issue: #87932

@liveans liveans closed this as completed Jun 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Projects
None yet
Development

No branches or pull requests

10 participants