Implement Socket.DuplicateAndClose() for Windows #1858

antonfirsov · 2020-01-17T15:27:11Z

The implementation is based on Framework code, with the following differences:

Breaking change: When trying to bind a socket to a ThreadPool (thus IOCP port), if the socket has been previously bound to IOCP, an InvalidOperationException is thrown, instead of an undocumented ArgumentException.
SocketInformation is not [Serializable] and does not contain a snapshot of RemoteEndpoint. Instead, the endpoint is being restored using getsockname in all cases.

alnikola · 2020-01-17T16:18:19Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs

+                    _rightEndPoint = ep.Create(socketAddress);
+                }
+                catch
+                {


It looks suspicious that we silently suppress all exceptions here. What is the motivation?

I seems to be a bad pattern inherited from the old framework code. Would make sense to replace IPEndPointExtensions.Create() with a non-throwing IPEndPointExtensions.TryCreate(), but I did not want to apply changes of that extent in this PR.

If this fails, the socket will be in a bad state. I'd rather we just let it throw.

That said, I don't know why this would fail as I'd expect socket duplication to not work if the address is wrong.

Doesn't that apply for all call sites? If we change the pattern, it's better to alter usages everywhere.
I would prefer to change it in a separate clean-up PR everywhere rather than breaking the (bad) consistency on a single call site.

Can you point to what other code you're comparing to? UDP would function mostly fine like this but to my knowledge this would be the first API where we can have a connected socket without an endpoint set, and the other common connection-oriented APIs (send/recv) don't set it. LocalEndPoint and RemoteEndPoint would both just be null forever.

Failing fast here would be better than failing down the line with a null endpoint, especially if it's "free" because the exception is already getting thrown.

The same exception swallowing pattern is present in Send/Receive implementations. I don't get the purpose of those calls though, since we are not doing anything with the remoteEp instances being returned.

Failing fast would be indeed better here, however the handling of (undocumented & impossiblish) exceptions coming from internals looks quite ad-hoc in existing code. Some call sites just let them leak out, others prefer to swallow them. Shouldn't be there a guideline point about this?

Removed the try-catch, but I'd really like to have better understanding. The code was taken from Framework.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs

alnikola · 2020-01-17T16:31:03Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs

+            Assert.Equal(blocking, clone.Blocking);
+        }
+
+        public class NotSupportedOnUnix


It's better not to nest one test class into another. It impairs readability.

A hierarchical pattern seems actually more readable for me in many cases: helps avoiding long test names, organizing stuff into groups instead. However in this particular case it would be definitely better to ungroup them.

I wonder if we have a written guideline for test code?

I don't think I've seen this pattern elsewhere in our tests.

Our unit testing is modeled after a book... I believe The Art of Unit Testing. @davidsh can confirm.

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketInformation.cs

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs

antonfirsov · 2020-01-17T18:51:22Z

I think I addressed most review findings, except the ones dealing with existing patterns (WSASocket return value and the dumb catch blocks).

Should I sneak fixes for those into this PR, or shall we do it separately instead?

src/libraries/System.Net.Sockets/src/System.Net.Sockets.csproj

scalablecory

a couple things, otherwise looks good.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketInformation.cs

antonfirsov · 2020-01-22T21:31:17Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-01-22T21:31:32Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotalik

Have we been able to pass these bits off to internal teams to verify?

jkotalik · 2020-01-22T22:11:00Z

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs

@@ -0,0 +1,52 @@
+using System;


nit: copyright

jkotalik · 2020-01-22T22:16:59Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs

+        {
+            socketInformation = new SocketInformation
+            {
+                ProtocolInformation = new byte[Interop.Winsock.WSAProtocolInfo.Size]


nit: stackalloc instead of newing up this array

This is an existing public API, that uses byte[].

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs

antonfirsov · 2020-01-27T14:43:46Z

Pushed a change to deal with handle inheritance. The solution is not trivial because of the esoteric behavior of WSASocket. I had to use SetHandleInformation. (See comments.)

@jkotalik thanks for your engagement here, it prevented a bug!

stephentoub · 2020-01-29T20:14:45Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs

@@ -59,6 +59,7 @@ internal ThreadPoolBoundHandle GetOrAllocateThreadPoolBoundHandle(bool trySkipCo
                    catch (Exception exception) when (!ExceptionCheck.IsFatal(exception))
                    {
                        bool closed = IsClosed;
+                        bool alreadyBound = !IsInvalid && !IsClosed && (exception is ArgumentException);


Won't we get an ArgumentException as well if the handle wasn't opened for overlapped I/O? We're assuming that's the case then?

I made this based this on my understanding of the code in ClrThreadPoolBoundHandle.Windows.cs:

runtime/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ClrThreadPoolBoundHandle.Windows.cs

Lines 30 to 34 in 6ed4a5e

if (ex.HResult == HResults.E_HANDLE) // Bad handle

throw new ArgumentException(SR.Argument_InvalidHandle, nameof(handle));

if (ex.HResult == HResults.E_INVALIDARG) // Handle already bound or sync handle

throw new ArgumentException(SR.Argument_AlreadyBoundOrSyncHandle, nameof(handle));

I assume that the handle can not be invalid since we checked it before. Based on the comment, and the exception message, the only other possible reason is: "Handle already bound or sync handle". Let me know, if I'm missing something!

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs

stephentoub · 2020-01-29T20:16:33Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs

+            //
+            // This constructor works in conjunction with DuplicateAndClose, which is not supported on Unix.
+            // See comments in DuplicateAndClose.
+            //


Do we use this style of commenting elsewhere in System.Net.Sockets, with a blank comment line above and below? We've generally moved away from that elsewhere.

Occasionally yes, it was was used in the methods I changed. Getting rid of it.

stephentoub · 2020-01-29T20:17:50Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs

+            }
+
+            if (_handle.IsInvalid)
+            {


We should dispose of state here that may have been created, namely the _handle. Even if it's invalid, it'll likely still trigger finalization. Probably want to null it out as well.

This looks like an unnecessary check, since SocketPal.CreateSocket already validates handle. Added the disposal there, getting rid of this branch here.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs

default System.Net.Sockets.sln configurations to Windows_NT again

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.HandleInformation.cs

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs

stephentoub · 2020-02-03T16:54:26Z

src/libraries/Common/tests/System/Net/Sockets/SocketTestExtensions.cs

+
+        public static (Socket, Socket) CreateConnectedSocketPair()
+        {
+            using Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);


Is there any potential difference in the handling of different address families? Wondering if we need tests for IPv6 sockets as well, or if Windows doesn't discriminate and a test for IPv4 sockets will be sufficient.

To me it looks quite arbitrary whether a functionality is also tested against IPV6 in socket test code. I added a few IPV6 test cases for the duplication to make sure it is covered. See the parameters of DuplicateAndClose_TcpServerHandler which is emulating most common user scenario.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs

stephentoub · 2020-02-03T17:17:45Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs

+            Assert.Equal(TestMessage, receivedMessage);
+        }
+
+        [OuterLoop] // long-running


What makes it long-running?

For the same reason CreateSocket.CtorAndAccept_SocketNotKeptAliveViaInheritance is running for about 2 sec in debug on my PC. There are nested child processes + pipes. I'm also wondering why is it that much.

We should figure out why it's taking 2 seconds. It shouldn't be that long.

Since this also concerns other tests, I'd rather take care of it later separately. I'm preparing a test fix PR anyways.

antonfirsov · 2020-02-03T19:49:10Z

src/libraries/System.Net.Sockets/System.Net.Sockets.sln

@@ -30,8 +30,8 @@ Global
 		{8CBA022C-635F-4C8D-9D29-CD8AAC68C8E6}.Debug|Any CPU.Build.0 = netcoreapp5.0-Windows_NT-Debug|Any CPU
 		{8CBA022C-635F-4C8D-9D29-CD8AAC68C8E6}.Release|Any CPU.ActiveCfg = netcoreapp5.0-Windows_NT-Release|Any CPU
 		{8CBA022C-635F-4C8D-9D29-CD8AAC68C8E6}.Release|Any CPU.Build.0 = netcoreapp5.0-Windows_NT-Release|Any CPU
-		{43311AFB-D7C4-4E5A-B1DE-855407F90D1B}.Debug|Any CPU.ActiveCfg = netcoreapp5.0-Unix-Debug|Any CPU
-		{43311AFB-D7C4-4E5A-B1DE-855407F90D1B}.Debug|Any CPU.Build.0 = netcoreapp5.0-Unix-Debug|Any CPU
+		{43311AFB-D7C4-4E5A-B1DE-855407F90D1B}.Debug|Any CPU.ActiveCfg = netcoreapp5.0-Windows_NT-Debug|Any CPU


@stephentoub all other solutions default to Windows_NT. I suppose you altered this by accident in #2063.

If I modified it, it was VS modifying it automatically. I'd need to look at the various guids, but there is a unix build in this project, so it would make sense for there to be unix entries here.

antonfirsov · 2020-02-04T01:26:58Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-02-04T01:27:11Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov added 2 commits January 17, 2020 16:02

reimplement socket duplication, fix dotnet#1760

52dc552

additional cleanup

5b617e6

Dotnet-GitSync-Bot added the area-System.Net.Sockets label Jan 17, 2020

antonfirsov requested review from a team and scalablecory January 17, 2020 15:27

alnikola reviewed Jan 17, 2020

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs Show resolved Hide resolved

alnikola reviewed Jan 17, 2020

View reviewed changes

ungroup PNSE tests, no RemoteExecutor timeout

bee3140

scalablecory reviewed Jan 17, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 17, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 17, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 17, 2020

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketInformation.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 17, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSADuplicateSocket.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jan 17, 2020

View reviewed changes

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs Show resolved Hide resolved

address review findings

34bf608

antonfirsov added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jan 17, 2020

ManickaP reviewed Jan 20, 2020

View reviewed changes

src/libraries/System.Net.Sockets/src/System.Net.Sockets.csproj Outdated Show resolved Hide resolved

antonfirsov added 2 commits January 20, 2020 18:17

fix path for Interop.WSADuplicateSocket.cs

fe81eeb

remove swallowing in Socket(SocketInformation)

23fdf75

scalablecory approved these changes Jan 21, 2020

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs Outdated Show resolved Hide resolved

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketInformation.cs Outdated Show resolved Hide resolved

antonfirsov added 2 commits January 22, 2020 14:13

Merge branch 'master' into af/duplicate-socket

a0ff6f3

review suggestions

3d801cc

jkotalik reviewed Jan 22, 2020

View reviewed changes

make sure duplicate socket is not inheritable

54df6d9

remove debug code leftover

7ea829a