System.Net.Socket EndPoint-extensibility broken in corefx

[stephentoub]

The current corefx codebase appears to be closed off to any EndPoint customization. The Sockets codebase expects every EndPoint to be either a DnsEndPoint or an IPEndPoint, and anything that’s not built in to System.Net.Sockets will result in failures. This is a silly little quick example which obviously won’t do anything meaningful:

private class MyEndPoint : EndPoint { }

[Fact]
public void Socket_CustomEndPoint()
{
    Socket s = new Socket(SocketType.Raw, ProtocolType.Raw);
    s.Connect(new MyEndPoint());
}

but it fails on corefx due to a cast exception that highlights the problem:

System.InvalidCastException : Unable to cast object of type 'MyEndPoint' to type 'System.Net.IPEndPoint'.
        Stack Trace:
           c:\Users\stoub\Source\repos\corefx\src\Common\src\System\Net\Internals\IPEndPointExtensions.cs(14,0): at System.Net.Sockets.IPEndPointExtensions.Serialize(EndPoint endpoint)
           c:\Users\stoub\Source\repos\corefx\src\System.Net.Sockets\src\System\Net\Sockets\Socket.cs(4894,0): at System.Net.Sockets.Socket.CheckCacheRemote(EndPoint& remoteEP, Boolean isOverwrite)
           c:\Users\stoub\Source\repos\corefx\src\System.Net.Sockets\src\System\Net\Sockets\Socket.cs(848,0): at System.Net.Sockets.Socket.Connect(EndPoint remoteEP)

In contrast, on desktop it also fails but where and with the exception I’d expect:

Unhandled Exception: System.NotImplementedException: This method is not implemented by this class.
   at System.Net.EndPoint.Serialize()
   at System.Net.Sockets.Socket.CallSerializeCheckDnsEndPoint(EndPoint remoteEP)
   at System.Net.Sockets.Socket.CheckCacheRemote(EndPoint& remoteEP, Boolean isOverwrite)
   at System.Net.Sockets.Socket.Connect(EndPoint remoteEP)

cc: @CIPop, @davidsh, @SidharthNabar

[IlyaBiryukov]

It seems that the root cause of that issue is that System.Net.Sockets.dll has its own SocketAddress class and doesn’t use the one from System.dll. So it cannot rely on System.Net.IPEndPoint.Serialize() which returns System.Net.SocketAddress from System.dll, and has to use IPEndPointExtensions that return the SocketAddress from System.Net.Sockets when it needs to serialize the end point, basically circumventing EndPoint.Serialize() method.

A naïve solution would be to fallback to EndPoint.Serialize() if it is not IPEndPoint, but that will return different SocketAddress, the one from System.dll. So comes the question, why we cannot use it from System.dll in the first place? Why do we have to have the our own SocketAddress in System.Net.Sockets.dll?

[stephentoub]

why we cannot use it from System.dll in the first place?

System.dll doesn't exist on coreclr.

[IlyaBiryukov]

Hmm. That must be language services tricked me in VS when I tried to find the definition of SocketAddress in IPEndPoint.Serialize() in System.Net.Primitives.dll. I looked up SocketAddress and it is indeed in System.Net.Primitives.dll

But I guess the question still stands, why use it from System.Net.Sockets, why not the original one that EndPoint.Serialize() returns, from System.Net.Primitives?

[IlyaBiryukov]

The root cause of the issue is that System.Net.Sockets has a copy of SocketAddress class from System.Net.Primitives to get access to its internal members. As EndPoint members return SocketAddress from System.Net.Primitives, and Socket class uses SocketAddress from System.Net.Sockets, the EndPoint extensibility is broken.

One possible fix is to make the members used by Socket public on SocketAddress in System.Net.Primitives

[CIPop]

The root cause of the issue is that System.Net.Sockets has a copy of SocketAddress class from System.Net.Primitives to get access to its internal members. As EndPoint members return SocketAddress from System.Net.Primitives, and Socket class uses SocketAddress from System.Net.Sockets, the EndPoint extensibility is broken.

That’s correct: using the System.Net.Internal namespace has been the only solution I’ve seen when we were initially porting things to GitHub under pressure from various deadlines.

(From the offline discussion: ) A possible solution would be to add InternalsVisibleToAttribute to System.Net.Internals. Hough I don’t see much use of InternalsVisibleTo attribute in CoreFx.

We definitely don’t want any InternalsVisibleTo or reflection. Not only it’s bad practice but it may create issues in NetNative.

One possible fix is to make the members used by Socket public on SocketAddress in System.Net.Primitives

Making the members public is the only option I see right now. Unfortunately that requires changes to the .Net Desktop API as well: the contracts are part of generations which span across .Net SKUs (such as K, N, Desktop, Mono, etc).

@ericstj @weshaggard @stephentoub I’m not sure we are allowed to add public API changes at this point for .Net 4.6.1 and so we can't have a fix for this for RTM (or maybe in Primitives 4.1.0 / Sockets 4.1.0). If my understanding of generations/versions is correct, we could increment the contract version after we release 4.1 and add the new APIs in the 4.2 versions.

@pgavlin One interesting issue would be to understand what publicly exposing a PAL-layer API means. I expect that the struct address format will be different across different platforms.

[IlyaBiryukov]

I think we can do that without adding new public area.

The purpose of the fix is to make Socket respect SocketAddress that a custom EndPoint returns. Right now it only respects the one from the same assembly.
For IPEndPoint the conversion is taken care of by IPEndPointExtensions. DnsEndPoint is also treated differently by Socket.

The fix may be to be able to convert System.Net.Primitives SocketAddress to System.Net.Socket SocketAddress and back if it is not IP or DNS end point.

There are also places where we need to create the end point from the SocketAddress. There we'd need to create System.Net.Primitives SocketAddress
from System.Net.Sockets SocketAddress, and then create the EndPoint from that.

There are these public members on SocketAddress that can help us with conversion:

AddressFamily Family { get; }
int Size { get; }
byte this[int offset] { get; set; }
SocketAddress(AddressFamily family, int size)

There is one piece of data that is not accessible though, it is set by CopyAddressSizeIntoBuffer() and to access it, GetAddressSizeOffset() and internal Buffer are used.
This is used only by ReceiveMessageOverlappedAsyncResult. It seems that we won't be blocked by that because the System.Net.Sockets SocketAddress used by
ReceiveMessageOverlappedAsyncResult is pinned for native code and doesn't need to round-trip with System.Net.Primitives SocketAddress.

Iterating over an array has some perf cost, but that will only be paid for new types of end points and is negligible compared to networking costs. Existing scenarios should not be affected.

[CIPop]

Iterating over an array has some perf cost, but that will only be paid for new types of end points and is negligible compared to networking costs. Existing scenarios should not be affected.

Sounds good. Please send a PR.