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

[API Proposal]: add overloads with SocketAddress to Socket's SendTo and ReceiveFrom #87397

Closed
wfurt opened this issue Jun 12, 2023 · 11 comments · Fixed by #90086
Closed

[API Proposal]: add overloads with SocketAddress to Socket's SendTo and ReceiveFrom #87397

wfurt opened this issue Jun 12, 2023 · 11 comments · Fixed by #90086
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Jun 12, 2023

Background and motivation

This is subset from #30797. There is long discussion about how to make sending and receiving UDP more efficient. Current overloads allocate a lot for each packet and that kills performance and it puts lot of pressure on GC. Garbage collection has significant impact on scenarios that are sensitive to timing - like multiplayer Unity games.

API Proposal

namespace System.Net.Sockets;

public class Socket
{
+  public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
+  public ValueTask<int> ReceiveFromAsync(Memory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);

+. public int SendTo(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
+. public ValueTask<int> SendToAsync(ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);
}

Instead of allocating and returning EndPoint, we can reuse SocketAddress and we would update it in place. Ideally using API proposed in #86872 but in worst case the SocketAddress is already writable and we can play some internal tricks.

API Usage

example of simple server that would process request - inspired by DNS server linked in #30797

  SocketAddress sa = new SocketAddress(AddressFamily.InterNetworkV6);
  while (doWork)
  {
      int receivedLength = s.ReceiveFrom(buffer, SocketFlags.None, sa);
      response = processRequest(buffer.Slice(0, receivedLength);
      s.SendTo(response, SocketFlags.None, sa);
  }

The async version would be basically same with addition of CancellationToken. Unlike ReceiveFromAsync that used EndPoint and returns SocketReceiveFromResult we would return ValueTask<int> just like the synchronous version and we would update socketAddress in-place.

Alternative Designs

No response

Risks

This is low-level API intended for experienced users. While there is nothing particularly dangerous one should be very careful about life cycle of the socketAddress as it can be overwritten on subsequent use.

@wfurt wfurt added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 12, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 12, 2023
@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Jun 12, 2023
@ghost
Copy link

ghost commented Jun 12, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

This is subset from #30797. There is long discussion about how to make sending and receiving UDP more efficient. Current overloads allocate a lot for each packet and that kills performance and it puts lot of pressure on GC. Garbage collection has significant impact on scenarios that are sensitive to timing - like multiplayer Unity games.

API Proposal

namespace System.Net.Sockets;

public class Socket
{
+  public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
+  public ValueTask<int> ReceiveFromAsync(Memory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);

+. public int SendTo(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
+. public ValueTask<int> SendToAsync(ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);
}

Instead of allocating and returning EndPoint, we can reuse SocketAddress and we would update it in place. Ideally using API proposed in #86872 but in worst case the SocketAddress is already writable and we can play some internal tricks.

API Usage

example of simple server that would process request - inspired by DNS server linked in #30797

  SocketAddress sa = new SocketAddress(AddressFamily.InterNetworkV6);
  while (doWork)
  {
      int receivedLength = s.ReceiveFrom(buffer, SocketFlags.None, sa);
      response = processRequest(buffer.Slice(0, receivedLength);
      s.SendTo(response, SocketFlags.None, sa);
  }

The async version would be basically same with addition of CancellationToken. Unlike ReceiveFromAsync that used EndPoint and returns SocketReceiveFromResult we would return ValueTask<int> just like the synchronous version and we would update socketAddress in-place.

Alternative Designs

No response

Risks

This is low-level API intended for experienced users. While there is nothing particularly dangerous one should be very careful about life cycle of the socketAddress as it can be overwritten on subsequent use.

Author: wfurt
Assignees: -
Labels:

api-suggestion, area-System.Net.Sockets

Milestone: -

@wfurt wfurt added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 12, 2023
@wfurt wfurt added this to the 8.0.0 milestone Jun 12, 2023
@wfurt wfurt added the blocking Marks issues that we want to fast track in order to unblock other important work label Jul 6, 2023
@terrajobst
Copy link
Member

terrajobst commented Jul 13, 2023

Video

  • We don't like ReceiveFrom because it mutates the socket address
namespace System.Net.Sockets;

public partial class Socket
{
    // public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
    // public ValueTask<int> ReceiveFromAsync(Memory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);

    public int SendTo(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
    public ValueTask<int> SendToAsync(ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 13, 2023
@fredrikhr
Copy link
Contributor

Thinking about the ergonomics of receiving the SocketAddress I think the following proposal could make ReceiveFrom viable:

This basically adds ref for the SocketAddress parameter.

namespace System.Net.Sockets;

public partial class Socket
{
    public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, ref SocketAddress socketAddress);
    public ValueTask<int> ReceiveFromAsync(Memory<byte> buffer, SocketFlags socketFlags, ref SocketAddress socketAddress, CancellationToken cancellationToken = default);
}

@antonfirsov
Copy link
Member

This has been considered, the problem is that ref doesn't play well with async.

@fredrikhr
Copy link
Contributor

fredrikhr commented Jul 13, 2023

@antonfirsov can you elaborate on that? I just tried the signature with this in SharpLab https://sharplab.io/#gist:14ff035d0a0dec97365a6d992b47dfd9. What am I missing?

@antonfirsov
Copy link
Member

ReceiveFromAsync would be only able to only mutate it on it's synchronous path. If you mark the method async, the compiler will complain. As a rule of thumb, async methods never take ref.

@fredrikhr
Copy link
Contributor

Thanks for indulging in my ignorance. Makes perfect sense now! I am excited to see Levi's idea that he started to propose during API review.

@stephentoub
Copy link
Member

stephentoub commented Jul 15, 2023

We don't like ReceiveFrom because it mutates the socket address

Why is it a problem to mutate the SocketAddress? The whole point of this was to mutate it, thereby providing a way of achieving an allocation-free receive path. We have other methods that mutate arguments... why can't this one?

SocketAddress is just a strongly-typed buffer. Mutating buffers in read/receive methods is how things work.

@dersia
Copy link

dersia commented Jul 17, 2023

We don't like ReceiveFrom because it mutates the socket address

Why is it a problem to mutate the SocketAddress? The whole point of this was to mutate it, thereby providing a way of achieving an allocation-free receive path. We have other methods that mutate arguments... why can't this one?

SocketAddress is just a strongly-typed buffer. Mutating buffers in read/receive methods is how things work.

during the meeting it came up that users might assume that ReceiveFrom would receive an address with the parameter and not expect for it to mutate. I general the problem wasn't that it is mutating, but that the name was misleading and therefor it was excluded for now.

at least that is what I understood from the meeting.

@stephentoub
Copy link
Member

Thanks. To me that's an issue for documentation: the overload may write into the supplied buffers, both for the data received and for the address from which it was received.

I'd really like for these overloads to be in .NET 8. @bartonjs, @terrajobst, can we revisit this at the beginning of the next API review?

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation api-approved API was approved in API review, it can be implemented and removed api-approved API was approved in API review, it can be implemented api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 18, 2023
@terrajobst
Copy link
Member

terrajobst commented Jul 18, 2023

Video

  • let's change the name of the parameter socketAddress to receivedAddress to make it clear that it will be mutated.
namespace System.Net.Sockets;

public partial class Socket
{
    public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, SocketAddress receivedAddress);
    public ValueTask<int> ReceiveFromAsync(Memory<byte> buffer, SocketFlags socketFlags, SocketAddress receivedAddress, CancellationToken cancellationToken = default);

    // Approved already
    // public int SendTo(ReadOnlySpan<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress);
    // public ValueTask<int> SendToAsync(ReadOnlyMemory<byte> buffer, SocketFlags socketFlags, SocketAddress socketAddress, CancellationToken cancellationToken = default);
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 7, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 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 blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants