Support UnixDomainSocketEndPoint on Windows #27631

stephentoub · 2018-03-01T23:02:44Z

Windows is adding support for Unix domain sockets, and it's easy to enable that support via our new UnixDomainSocketEndPoint type.

Fixes https://github.com/dotnet/corefx/issues/27542
cc: @eerhardt, @danmosemsft, @geoffkizer, @sunilmut, @tmds

stephentoub · 2018-03-01T23:03:17Z

src/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.Unix.cs

@@ -28,108 +22,5 @@ static UnixDomainSocketEndPoint()
            Debug.Assert(s_nativePathOffset + s_nativePathLength <= s_nativeAddressSize, "Expected address size to include all of the path length");
            Debug.Assert(s_nativePathLength >= 92, "Expected max path length to be at least 92"); // per http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_un.h.html
        }
-
-        public UnixDomainSocketEndPoint(string path)


All of this code just moved to the other partial class, with very few changes. I'll point them out individually.

stephentoub · 2018-03-01T23:03:33Z

src/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

+            if (!s_udsSupported.Value)
+            {
+                throw new PlatformNotSupportedException();
+            }


This check is obviously new.

Would it make sense to do this at the beginning of the constructor?

In reply to: 171722160 [](ancestors = 171722160)

My thinning was that we want to make sure all the arguments are valid regardless of whether support is currently there. I could go either way, though.

Ok, I can understand that reasoning. Just wanted to make sure.

stephentoub · 2018-03-01T23:04:29Z

src/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

+
+        public override SocketAddress Serialize()
+        {
+            var result = new SocketAddress(AddressFamily.Unix, s_nativeAddressSize);


The length changed from s_nativePathOffset + _encodedPath.Length to s_nativeAddressSize.

Why the change?

In reply to: 171722363 [](ancestors = 171722363)

Because it needs to match the native type in size/layout; otherwise if the native implementation dereferences, it can AV and other such things, and the native implementation may also do a size check. Basically it's dangerous to use anything smaller than the corresponding native definition.

stephentoub · 2018-03-01T23:05:08Z

src/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.netcoreapp.cs

                        {
+                            bytesRead = await client.ReceiveAsync(new ArraySegment<byte>(buffer), SocketFlags.None);
+                            Assert.InRange(bytesRead, 1, writeBuffer.Length - readData.Length);


I changed this because in my build there are some issues with shutdown, and the test would sometimes hang.

@stephentoub - For the hang, as discussed offline, couple of similar bugs were fixed in the release branch. If you can give it a try on a recent build.

If you can give it a try on a recent build.

I will, thanks.

geoffkizer · 2018-03-01T23:12:51Z

Did you punt on the SAEA issue?

geoffkizer · 2018-03-01T23:13:49Z

Should we add a static OSSupportsUnixDomainSockets, like OSSupportsIPv4/6?

stephentoub · 2018-03-02T00:13:49Z

@dotnet/dnceng, can you help me to understand what failed in https://ci3.dot.net/job/dotnet_corefx/job/master/job/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/9488/consoleText? Some kind of infrastructure issue?

stephentoub · 2018-03-02T00:15:13Z

Did you punt on the SAEA issue?

Yes. There's an easy workaround if you think it's worthwhile, which is to essentially just do the same thing that's done in the other asynchronous connects and fall back to a synchronous connect for things other than DnsEndPoint and IPEndPoint.

Should we add a static OSSupportsUnixDomainSockets, like OSSupportsIPv4/6?

I think that'd be reasonable. We can open an issue.

davidfowl · 2018-03-02T06:19:47Z

src/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.Unix.cs

@@ -8,18 +8,12 @@
 namespace System.Net.Sockets
 {
    /// <summary>Represents a Unix Domain Socket endpoint as a path.</summary>
-    public sealed class UnixDomainSocketEndPoint : EndPoint
+    public sealed partial class UnixDomainSocketEndPoint : EndPoint


I had no idea we made this public

eerhardt · 2018-03-02T15:39:48Z

src/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

+
+            // Pathname socket addresses should be null-terminated.
+            // Linux abstract socket addresses start with a zero byte, they must not be null-terminated.
+            bool isAbstract = IsAbstract(path);


Does Windows support abstract socket addresses? Will it fail later if one is passed in?

Oh, I see the test now. Nice!

Yes, Windows unix sockets supports abstract addresses, but, it does not support the autobind feature available in linux (see man7 unix).

eerhardt · 2018-03-02T15:45:56Z

src/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.netcoreapp.cs

+            // Some Windows will.
+            try
+            {
+                new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified).Dispose();


This assumes that our product code is doing the right thing. Would it be better to P/Invoke into Windows directly to see if it is supported?

Or is their some other way to ask Windows if it is supported?

To check whether a given socket domain, type or protocol combo is supported, the best thing is to call the socket routine and check the return value.

To check whether a given socket domain, type or protocol combo is supported, the best thing is to call the socket routine and check the return value.

That's essentially what this code is doing, just via the .NET Socket type.

But to Eric's point, sure, I can just P/Invoke directly.

eerhardt · 2018-03-02T15:47:22Z

src/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.netcoreapp.cs

@@ -443,5 +442,29 @@ private static string GetRandomNonExistingFilePath()

            return result;
        }
+
+        private static bool PlatformDoesntSupportUnixDomainSockets => s_platformSupportsUnixDomainSockets.Value;


Should there be a ! here?

Yes, that's why the Windows CI failed. :)

MattGal · 2018-03-02T18:22:23Z

@stephentoub we had a total outage of telemetry processing, which is resolved now but was causing the Helix API to not reflect recently started jobs. Once the Jenkins plugin waited 15 minutes, it failed with the java.lang.NullPointerException as you noted (generally the wait between sending work and showing up in the API is < 5 seconds). This was dotnet/core-eng#2805.

Windows is adding support for Unix domain sockets, and it's easy to enable that support via our new UnixDomainSocketEndPoint type.

* Support UnixDomainSocketEndPoint on Windows Windows is adding support for Unix domain sockets, and it's easy to enable that support via our new UnixDomainSocketEndPoint type. * Address PR feedback Commit migrated from dotnet/corefx@421ce7b

stephentoub commented Mar 1, 2018

View reviewed changes

davidsh added the area-System.Net.Sockets label Mar 1, 2018

stephentoub closed this Mar 2, 2018

stephentoub reopened this Mar 2, 2018

davidfowl reviewed Mar 2, 2018

View reviewed changes

ahsonkhan assigned stephentoub Mar 2, 2018

eerhardt reviewed Mar 2, 2018

View reviewed changes

Support UnixDomainSocketEndPoint on Windows

40e84a9

Windows is adding support for Unix domain sockets, and it's easy to enable that support via our new UnixDomainSocketEndPoint type.

stephentoub force-pushed the udswindows branch 2 times, most recently from 903f3cf to afeb431 Compare March 2, 2018 19:52

Address PR feedback

17f2326

stephentoub force-pushed the udswindows branch from afeb431 to 17f2326 Compare March 2, 2018 21:23

stephentoub merged commit 421ce7b into dotnet:master Mar 2, 2018

stephentoub deleted the udswindows branch March 2, 2018 23:28

karelz added this to the 2.1.0 milestone Mar 10, 2018

CXuesong mentioned this pull request May 2, 2018

Support Unix Domain Socket CXuesong/JsonRpc.Standard#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support UnixDomainSocketEndPoint on Windows #27631

Support UnixDomainSocketEndPoint on Windows #27631

stephentoub commented Mar 1, 2018 •

edited

Loading

stephentoub Mar 1, 2018

stephentoub Mar 1, 2018

eerhardt Mar 2, 2018

stephentoub Mar 2, 2018

eerhardt Mar 2, 2018

stephentoub Mar 1, 2018

eerhardt Mar 2, 2018

stephentoub Mar 2, 2018

stephentoub Mar 1, 2018

sunilmut Mar 2, 2018

stephentoub Mar 2, 2018

geoffkizer commented Mar 1, 2018

geoffkizer commented Mar 1, 2018

stephentoub commented Mar 2, 2018

stephentoub commented Mar 2, 2018

davidfowl Mar 2, 2018

eerhardt Mar 2, 2018

stephentoub Mar 2, 2018

eerhardt Mar 2, 2018

sunilmut Mar 2, 2018 •

edited

Loading

eerhardt Mar 2, 2018 •

edited

Loading

sunilmut Mar 2, 2018

stephentoub Mar 2, 2018

eerhardt Mar 2, 2018

stephentoub Mar 2, 2018

MattGal commented Mar 2, 2018

Support UnixDomainSocketEndPoint on Windows #27631

Support UnixDomainSocketEndPoint on Windows #27631

Conversation

stephentoub commented Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Mar 1, 2018

geoffkizer commented Mar 1, 2018

stephentoub commented Mar 2, 2018

stephentoub commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunilmut Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

eerhardt Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattGal commented Mar 2, 2018

stephentoub commented Mar 1, 2018 •

edited

Loading

sunilmut Mar 2, 2018 •

edited

Loading

eerhardt Mar 2, 2018 •

edited

Loading