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]: make SocketAddress more useful #86872

Closed
wfurt opened this issue May 29, 2023 · 9 comments · Fixed by #88970
Closed

[API Proposal]: make SocketAddress more useful #86872

wfurt opened this issue May 29, 2023 · 9 comments · Fixed by #88970
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 May 29, 2023

Background and motivation

The SocketAddress exists for long time but it is pretty useless at this point. It allows to Create and Serialize EndPoint to buffer that is passable to operation system and it allows read/write access to underlying buffer via indexing.

However, as noted in #78757 there is no convenient access via Span or Memory and there is no way how to pin the buffer so it can actually (via handle or fixed) be passable to OS call.

To solve this limitation, Socket code use Internals.SocketAddress and there are extra steps and allocations to convert from EndPoint to buffer and we do sometimes extra processing to covert Internals.SocketAddress to SocketAddress to get the actual EndPoint. That brings unnecessary allocations and work to core Socket functions. It would be really nice to avoid all this and simply use public API on SocketAddress to do everything we need. While #78993 would help in many cases Socket code also needs to deal with dual mode sockets and mapping between IP address families.

Note that SocketAddress lives in System.Net.Primitives and System.Net.Socket code does not have convenient access to internal methods.

To be able to handle UDP processing efficiently, #30797 is proposing to add overloads with SocketAddress and it would be nice to expose some of the functionality that Socket currently has.

Lastly, #86513 noted some issues around using GCHandle. It would be nice to be able to use NativeMemory in some cases to avoid pinning in managed code.

API Proposal

namespace System.Net;

-public class SocketAddress
+public class SocketAddress : IEquatable<SocketAddress>
{
-     public int Size { get; }
+     public int Size { get; set; }
+     public static int GetMaximumAddressSize(AddressFamily addressFamily);
+     public System.Memory<byte> SocketBuffer { get; set; }
}

This is optional part to make some convenience helper functions. For Socket refactoring we can use private attic extension e.g. ability to have efficient access to underlying data and ability to shrink given buffer to actual addresses is sufficient.

SocketAddress already has Equals method e.g. the IEquatable is just to make it more obvious. Currently, it can only find equality with another SocketAddress. We could also make it work for known EndPoint type and use allocating Serialize method to SocketAddress as fallback.

namespace System.Net;

public class SocketAddress
{

+     public bool TryGetAddress(out System.Int128 address, out long scopeid) { throw null; }
+     public bool TryGetPort(out int port) { throw null; }
+     public bool TryWriteAddressBytes(Span<byte> destination, out int bytesWritten) { throw null; }
}

API Usage

UDP server

SocketAddress sa = new SocketAddress(socket.AddressFamily)
byte[] buffer = new byte[MaxBuffer];
while (true)
{
    int len = socket.ReceiveFrom(buffer, SocketFlags.None, sa);
    if (len > 0) 
    {
        ProcessMessage(buffer, sa.SocketBuffer.Span);
    } 
}

unsafe void ProcessMessage(byte[] buffer, readOnlySpan<byte> socketAddressBuffer)
{ 
  fixed (void *ptr = socketAddressBuffer)
  {
       p/invoke to native with ptr
  }
   ... 
   ... 
}

UDP client

   socket.ReceiveFrom(rawReceiveBuffer, buffer, newClientSA);
   newClientSA.TryGetPort(out int port);
   newClientSA. TryGetAddress(out int128 address, out long scope);
   var connectionId = Common.GetId(address, scope, port);

Crucial part is ability to get/set the bytes efficiently and ability to shrink the address. The current assumption is that setting Size would not allow to set it to something bigger than allocated buffer e.g. string the address.

on DualMode Socket we can receive IPv4 or IPv6 address. We need space for either one but at the end we may need less than allocated. It may be OK to pass more data to Create method but it feels it would be nice to have the prices length as the OS functions would typically provide that.

SocketAddress sa = new SocketAddress(AddressFamily. InterNetworkV6)
/// native receive call ....
if (sa.Family ==  AddressFamily.InterNetwork)
{
  sa.Size = GetSocketAddressSize(AddressFamily. InterNetwork);
}

TryGetPort make sense only IP protocol at the moment. TryGetAddress is similar but it may return something in future - like for VSock or Netlink where the address is process ID.

TryWriteAddressBytes follow similar similar method on IPAddress. for UnixDomainSocket it would return the sun_path of sockaddr_un e.g. it can be useful even in cases when address is not in numerical form.

All three method above are for convenience and could be implemented externally if needed. While we already have implementation and IP protocol seems vastly prevalent they can be useful for anybody who wants to avoid allocation of IPEndPoint just to do logging or some trivial functionality.

Alternative Designs

As noted above, #78993 may be good alternative in many cases. However it would not be sufficient for the new UDP methods as that plan is to avoid EndPoint completely as well as it does help with mappings for dual mode.

Risks

The primarily goal is to provide sufficient surface that we already have internally.

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

ghost commented May 29, 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

The SocketAddress exists for long time but it is pretty useless at this point. It allows to Create and Serialize EndPoint to buffer that is passable to operation system and it allows read/write access to underlying buffer via indexing.

However, as noted in #78757 there is no convenient access via Span or Memory and there is no way how to pin the buffer so it can actually (via handle or fixed) be passable to OS call.

To solve this limitation, Socket code use Internals.SocketAddress and there are extra steps and allocations to convert from EndPoint to buffer and we do sometimes extra processing to covert Internals.SocketAddress to SocketAddress to get the actual EndPoint. That brings unnecessary allocations and work to core Socket functions. It would be really nice to avoid all this and simply use public API on SocketAddress to do everything we need. While #78993 would help in many cases Socket code also needs to deal with dual mode sockets and mapping between IP address families.

Note that SocketAddress lives in System.Net.Primitives and System.Net.Socket code does not have convenient access to internal methods.

To be able to handle UDP processing efficiently, #30797 is proposing to add overloads with SocketAddress and it would be nice to expose some of the functionality that Socket currently has.

Lastly, #86513 noted some issues around using GCHandle. It would be nice to be able to use NativeMemory in some cases to avoid pinning in managed code.

API Proposal

namespace System.Net;

public class SocketAddress
{
-     public int Size { get { throw null; } }
+     public int Size { get { throw null; } set { } }
+     public static int GetSocketAddressSize(AddressFamily addressFamily)  { get { throw null; } }
+     public System.Memory<byte> SocketBuffer { get { throw null; } set { } }
+     public bool TryGetAddress(out System.Int128 address, out long scopeid) { throw null; }
+     public bool TryGetPort(out int port) { throw null; }
+     public bool TryWriteAddressBytes(Span<byte> destination, out int bytesWritten) { throw null; }
}

API Usage

SocketAddress sa = new SocketAddress(socket.AddressFamily)
byte[] buffer = new byte[MaxBuffer];
while (true)
{
    int len = socket.ReceiveFrom(buffer, SocketFlags.None, sa);
    if (len > 0) 
    {
        ProcessMessage(buffer, sa.SocketBuffer.Span);
    } 
}

unsafe void ProcessMessage(byte[] buffer, readOnlySpan<byte> socketAddressBuffer)
{ 
  fixed (void *ptr = socketAddressBuffer)
  {
       p/invoke to native with ptr
  }
   ... 
   ... 
}

Crucial part is ability to get/set the bytes efficiently and ability to shrink the address. The current assumption is that setting Size would not allow to set it to something bigger than allocated buffer e.g. string the address.

on DualMode Socket we can receive IPv4 or IPv6 address. We need space for either one but at the end we may need less than allocated. It may be OK to pass more data to Create method but it feels it would be nice to have the prices length as the OS functions would typically provide that.

SocketAddress sa = new SocketAddress(AddressFamily. InterNetworkV6)
/// native receive call ....
if (sa.Family ==  AddressFamily.InterNetwork)
{
  sa.Size = GetSocketAddressSize(AddressFamily. InterNetwork);
}

TryGetPort make sense only IP protocol at the moment. TryGetAddress is similar but it may return something in future - like for VSock or Netlink where the address is process ID.

TryWriteAddressBytes follow similar similar method on IPAddress. for UnixDomainSocket it would return the sun_path of sockaddr_un e.g. it can be useful even in cases when address is not in numerical form.

All three method above are for convenience and could be implemented externally if needed. While we already have implementation and IP protocol seems vastly prevalent they can be useful for anybody who wants to avoid allocation of IPEndPoint just to do logging or some trivial functionality.

Alternative Designs

As noted above, #78993 may be good alternative in many cases. However it would not be sufficient for the new UDP methods as that plan is to avoid EndPoint completely as well as it does help with mappings for dual mode.

Risks

The primarily goal is to provide sufficient surface that we already have internally.

Author: wfurt
Assignees: -
Labels:

api-suggestion, area-System.Net.Sockets, untriaged

Milestone: -

@wfurt wfurt added this to the 8.0.0 milestone May 29, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 29, 2023
@MichalPetryka
Copy link
Contributor

AFAIR SocketAddress does not store a port, you need to use an IPEndPoint for that?

@wfurt
Copy link
Member Author

wfurt commented May 30, 2023

It does for IPv4/6: https://man7.org/linux/man-pages/man7/ipv6.7.html https://man7.org/linux/man-pages/man7/ip.7.html
and

internal int GetPort() => (int)SocketAddressPal.GetPort(Buffer);

@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
@antonfirsov
Copy link
Member

@wfurt the usage example only demonstrates the part of the proposed API that is intended to be consumed by end users. It would be more interesting to see how socket code implementing #87397 will interact with the new methods. Can you share your POC for that?

Some specific questions:

  • Do we really need a setter for SocketBuffer?
  • Would growing Size extend the buffer implicitly, and as a result invalidate all the Memory<byte> references to the previous buffer, or shall we better disallow such a case?

@wfurt
Copy link
Member Author

wfurt commented Jun 15, 2023

The setter for SocketBuffer is not needed for the internal socket refactor. My PoC moved everything from array to memory and uses Pin method when needed to. I envisioned somebody passing pre-pinned memory so we would save allocation for GCHandle/MemoryHandle for each operation for async processing. That goes above and beyond the original asks but it would help IMHO with memory management in certain cases.

And I don't plan to allow buffer growing. Attempt to grow size beyond buffer size would throw. We really only need to string it down in certain cases - like UDS.

@antonfirsov
Copy link
Member

envisioned somebody passing pre-pinned memory so we would save allocation for GCHandle/MemoryHandle for each operation for async processing.

Considering that we disallow growing the buffer, wouldn't it be then better to pass the Memory<byte> in a constructor in this use-case?

@wfurt
Copy link
Member Author

wfurt commented Jun 16, 2023

we could. I feel that the setter is more flexible but that is also something API review may care about. (there is already internal constructor that takes Span but that allocates the array internally and it it does data copy)

On the other hand, this is not critical for any work I plan for 8.0 so we can also take it out for now and do it later as needed.

@wfurt wfurt added the blocking Marks issues that we want to fast track in order to unblock other important work label Jul 6, 2023
@Rob-Hague
Copy link
Contributor

Rob-Hague commented Jul 13, 2023

One idea during the 13Jul API review of #87397 in which reservations were made about mutating the SocketAddress argument was to instead pass a nested type, the extra indirection highlighting the potential pitfalls present.

It seems like that could blend well with this API proposal.

public class SocketAddress
{
     // Remove from the proposal
-    public System.Memory<byte> SocketBuffer { get; set; }

+    public struct Buffer
+    {
+        public System.Memory<byte> Value { get; }
+        public SocketAddress Address { get; }
+    }

+    public SocketAddress.Buffer Buffer { get; }
}

the ReceiveFrom overloads then become akin to:

public int ReceiveFrom(Span<byte> buffer, SocketFlags socketFlags, SocketAddress.Buffer socketAddressBuffer);

@terrajobst
Copy link
Member

terrajobst commented Jul 18, 2023

Video

  • We should remove the setter for SocketBuffer and rename it to just Buffer
  • The setter for Size will throw if it's larger than SocketBuffer.Length.
  • The IEquatable<SocketAddress> doesn't change the equality contract, it just makes it callable via the interface
  • We discussed the helper methods but we don't like designing APIs for an internal consumer. Let's wait until we have a public consumer.
 namespace System.Net;
 
-public class SocketAddress
+public class SocketAddress : IEquatable<SocketAddress>
 {
-     public int Size { get; }
+     public int Size { get; set; }
+     public static int GetMaximumAddressSize(AddressFamily addressFamily);
+     public Memory<byte> Buffer { get; }
 }

@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 18, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 23, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 1, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 31, 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.

5 participants