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

add Span overloads for Socket datagram functions #938

Closed
wfurt opened this issue Mar 7, 2019 · 13 comments
Closed

add Span overloads for Socket datagram functions #938

wfurt opened this issue Mar 7, 2019 · 13 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Mar 7, 2019

Spanification, ValueTask, and cancellation

This proposal spanifies connectionless methods on Socket, uses a ValueTask return, and adds cancellation support. There are already such overloads in Send() and Receive() functions for connection oriented sockets (TCP) and this is complement for UDP.

Proposed APIs

namespace System.Net.Sockets
{
    // Open question: consider defaulting SocketFlags to SocketFlags.None to reduce API space? Might challenge IntelliSense.

    public partial class Socket : IDisposable
    {
        // 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);
    }

    public static class SocketTaskExtensions
    {
        // existing: public static Task<int> SendToAsync(this Socket socket, ArraySegment<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP);
        public static ValueTask<int> SendToAsync(this Socket socket, ReadOnlyMemory<byte> buffer, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<int> SendToAsync(this Socket socket, 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 static ValueTask<SocketReceiveFromResult> ReceiveFromAsync(this Socket socket, Memory<byte> buffer, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<SocketReceiveFromResult> ReceiveFromAsync(this Socket socket, 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 static ValueTask<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(this Socket socket, Memory<byte> buffer, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(this Socket socket, Memory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);
    }
}

Most of the underlying code is already ready, this is about exposing it as public API.

cc: @stephentoub

@wfurt wfurt self-assigned this Mar 7, 2019
@stephentoub
Copy link
Member

Thanks, @wfurt. What about the asynchronous equivalents? Do we want to have ValueTask<int>-based versions of these APIs that accept {ReadOnly}Memory<byte> as we do for ReceiveAsync and SendAsync?

@wfurt
Copy link
Member Author

wfurt commented Mar 8, 2019

I'm not sure. I look at SendAsync and ReceiveFromAsync as example and I only saw overload with SocketAsyncEventArgs. My general thinking was to mimic existing stream options.

@stephentoub
Copy link
Member

stephentoub commented Mar 9, 2019

@wfurt
Copy link
Member Author

wfurt commented Mar 12, 2019

updated. I missed that part when looking at Socket documentation. I was not sure if we also need overload for ReceiveMessageFrom(Async). With ipPacketInformation it probably won't be good fit for anybody looking for performance.

@hjc4869
Copy link

hjc4869 commented Mar 14, 2019

Is there any plan to avoid allocating EndPoint objects in these APIs? Since SendTo/ReceiveFrom APIs are called per datagram send/receive operation and they're probably on the hot path of a UDP server, even a small allocation could cause some kind of performance impact.

@karelz
Copy link
Member

karelz commented Apr 3, 2019

Not critical for 3.0, pushing to Future milestone.

@scalablecory
Copy link
Contributor

scalablecory commented Sep 9, 2019

Can we drop the two methods without SocketFlags? Users can specify SocketFlags.None. We have dropped these from our async APIs already:

    public partial class Socket : IDisposable
    {
        public int SendTo(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP);
        public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, ref EndPoint remoteEP);
    }

For async, these would be more consistent with existing Task-based APIs:

    public static class SocketTaskExtensions
    {
        public static ValueTask<int> SendToAsync(this Socket socket, ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<SocketReceiveFromResult> ReceiveFromAsync(this Socket socket, Memory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(this Socket socket, Memory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);
    }

(note return values to get endpoint / packet info, and defaulted cancellation token as last param)

@FiniteReality
Copy link

I also have a use case for these endpoints in some UDP code implementing a subset of RTP transmission, which runs on a 20ms timer. My data frames are less than 1kbytes total, and I believe implementing this as a Span operation would be much more efficient than my current implementation, which rents a buffer from ArrayPool<byte> to send instead.

@scalablecory
Copy link
Contributor

triage: maybe default the socket flags, and then ready for review.

@terrajobst
Copy link
Member

terrajobst commented Dec 10, 2019

Video

  • Looks good as proposed.
  • However, should we stop adding async methods as extension and just make them proper public instance methods?
  • In fact we may want to hide the entire SocketTaskExtensions because it was only used as a bridge pack for .NET Framework.
  • Remaining: we should have overloads for the method that take IList<ArraySegment<byte>> because they are more efficient.
namespace System.Net.Sockets
{
    // Open question: consider defaulting SocketFlags to SocketFlags.None to reduce API space? Might challenge IntelliSense.

    public partial class Socket : IDisposable
    {
        // 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);
    }

    public static class SocketTaskExtensions
    {
        // existing: public static Task<int> SendToAsync(this Socket socket, ArraySegment<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP);
        public static ValueTask<int> SendToAsync(this Socket socket, ReadOnlyMemory<byte> buffer, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<int> SendToAsync(this Socket socket, 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 static ValueTask<SocketReceiveFromResult> ReceiveFromAsync(this Socket socket, Memory<byte> buffer, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<SocketReceiveFromResult> ReceiveFromAsync(this Socket socket, 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 static ValueTask<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(this Socket socket, Memory<byte> buffer, EndPoint remoteEP, CancellationToken cancellationToken = null);
        public static ValueTask<SocketReceiveMessageFromResult> ReceiveMessageFromAsync(this Socket socket, Memory<byte> buffer, SocketFlags socketFlags, EndPoint remoteEP, CancellationToken cancellationToken = null);
    }
}

@maryamariyan maryamariyan transferred this issue from dotnet/corefx Dec 16, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Sockets untriaged New issue has not been triaged by the area owner labels Dec 16, 2019
@maryamariyan maryamariyan added the api-approved API was approved in API review, it can be implemented label Dec 16, 2019
@maryamariyan maryamariyan added this to the 5.0 milestone Dec 16, 2019
@antonfirsov antonfirsov self-assigned this Dec 19, 2019
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Jan 7, 2020
@ahsonkhan
Copy link
Member

Since we are adding span overloads to Socket, did we consider doing a full-pass on the type and adding similar overloads to the rest of the APIs on Socket?

For example, these APIs (+ overloads):

public IAsyncResult BeginReceive(byte[] buffer, int offset, int size, SocketFlags socketFlags, AsyncCallback callback, object state);
public IAsyncResult BeginReceiveMessageFrom(byte[] buffer, int offset, int size, SocketFlags socketFlags, ref EndPoint remoteEP, AsyncCallback callback, object state);
public IAsyncResult BeginSend(byte[] buffer, int offset, int size, SocketFlags socketFlags, out SocketError errorCode, AsyncCallback callback, object state);
public IAsyncResult BeginSendTo(byte[] buffer, int offset, int size, SocketFlags socketFlags, EndPoint remoteEP, AsyncCallback callback, object state);

@ohsorry
Copy link

ohsorry commented Feb 19, 2020

If we have Memory<T> version overloads in both TCP and UDP socket, I can design a system with such an interface:

    public partial interface IClient //regardless connection-oriented or connectionless.
    {        
        ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default);
        ValueTask<int> WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default);
    }

Pretty cool!

Can not use these two overloads in UDP if I don't have a default remote host:

public static ValueTask<int> ReceiveAsync(this Socket socket, Memory<byte> buffer, SocketFlags socketFlags, CancellationToken cancellationToken = default);

public static ValueTask<int> SendAsync(this Socket socket, ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, CancellationToken cancellationToken = default);

need a Memory<byte> version overload for ReceiveFromAsync() / SendToAsync().

@scalablecory
Copy link
Contributor

Closing for mega-issue #33418.

gigamon-dev added a commit to gigamon-dev/SubspaceServer that referenced this issue Sep 27, 2020
- BandwidthPriorities enum to not have a fake value for the length. Updated associated logic to use the maximum enum value + 1.
- Fixed logic that converts NetSendFlags to BandwidthPriorities.  It wasn't ignoring the Reiable/Droppable/Urgent flags properly.  Also, changed the logic so that it doesn't run for Ack and Reliable packets since those were overwriting the value anyway.
- Changed the 08 packets to be sent reliably.  This is different than what ASSS does.  I'm not sure if this is right and will need to investigate further.
- DumpPk method to output the text representation in its own column properly for lines that have < 16 bytes.
- Added an overload for SendToOne which takes a Span.  Found that Socket.SendTo does not allow passing a Span yet, but it looks like Microsoft may add it eventually (dotnet/runtime#938).  If they do, then it might be possible to switched everything from byte[] and ArraySegment<byte> to Span<byte>. Right now, the extra copy to a byte[] defeats the purpose.
@antonfirsov antonfirsov removed their assignment Nov 6, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
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