Socket: improve cross-platform behavior on Dispose

[tmds]

• Unix: unblock synchronous calls on Dispose
• Windows: implement unblock using CancelIoEx
• Unix/Windows: uses SafeHandle instead of IntPtr (needed on Windows for CancelIoEx unblocking)
• Unix: match when Windows sends RST on Close
• Unix/Windows: same SocketError for aborted operations

based on #37486

fixes #22564
fixes #26034

CC @wfurt @stephentoub @geoffkizer @davidsh @krwq

[tmds]

I'm first letting the tests of #37486 run on CI to ensure they are asserting current corefx behaviour on Windows.

[tmds]

I'm first letting the tests of #37486 run on CI to ensure they are asserting current corefx behaviour on Windows.

CI passes on Windows for all test except SyncSendFileGetsCanceledByDispose. For that test it fails because the operation isn't aborting on Dispose.

[davidsh]

CI passes on Windows for all test except SyncSendFileGetsCanceledByDispose. For that test it fails because the operation isn't aborting on Dispose.

Most of the CI test failures that we saw with the original PR were from Outerloop.

[davidsh]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[davidsh]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[tmds]

Based on reported issues for the previous PR this has changed:

• Cleanup the SO_TYPE socket length handling a1f790c
• Update SocketErrors when Cleanup for APM APIs 387542e. This was causing unexpected SocketError values for canceled operations.

[tmds]

CI failures:

• System.Tests.GetCommandLineArgs/GetCommandLineArgs_Invoke_ReturnsExpected(args: [\"\\\"Arg1 With Quotes\\\"\", \"\\\"Arg2 With Quotes\\\"\"]) failed on Windows nano:

Exit code was -1073740940 but it should have been 42
Expected: True
Actual:   False

• Catastrophic failure on Windows.7.Amd64.Open-x64 for System.Net.Http.Functional.Tests:

Process terminated. Assertion failed.
   at Interop.WinHttp.SafeWinHttpHandle.SetParentHandle(SafeWinHttpHandle parentHandle) in /_/src/Common/src/Interop/Windows/WinHttp/Interop.SafeWinHttpHandle.cs:line 35

   at System.Net.Http.WinHttpHandler.StartRequestAsync(WinHttpRequestState state) in /_/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs:line 813

   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine) in /_/src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncMethodBuilder.cs:line 1019

   at System.Net.Http.WinHttpHandler.StartRequestAsync(WinHttpRequestState state)

   at System.Net.Http.WinHttpHandler.<>c.<SendAsync>b__101_0(Object s) in /_/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs:line 560

   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state) in /_/src/System.Private.CoreLib/shared/System/Threading/ExecutionContext.cs:line 289

   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread) in /_/src/System.Private.CoreLib/shared/System/Threading/Tasks/Task.cs:line 2389

   at System.Threading.ThreadPoolWorkQueue.Dispatch() in /_/src/System.Private.CoreLib/shared/System/Threading/ThreadPool.cs:line 663
----- end Tue 06/25/2019  5:07:26.86 ----- exit code -2146232797 ----------------------------------------------------------

• mc.dot.net isn't showing results for Windows.10.Amd64.ClientRS5.

[tmds]

@davidsh @stephentoub @krwq @wfurt maybe we can merge this after a few more CI runs?

[stephentoub]

Is this going to make things worse than they are today? With this change, Dispose'ing of the associated SafeHandle will never unblock a synchronous Close/Accept, because the SafeHandle will have been add-ref'd. Might it have before this change, even if it was racy?

[tmds]

I think there are no regressions. I've opened #39079 to check how tests behaved before Socket changes.

[jkotalik]

We are seeing some issues with this change in Kestrel in the following test: https://github.com/aspnet/AspNetCore/blob/3a5dbad3bcf66750000943194f7270a2df3cc33d/src/Servers/Kestrel/test/FunctionalTests/ResponseTests.cs#L905. This is only failing on linux.

I'm not sure what specifically in this change caused the regression, but we saw this test fail ~30% of the time in Kestrel. More information here: dotnet/aspnetcore#13532

Here are the Kestrel logs we are seeing:

| [0.001s] TestLifetime Information: Starting test ClientCanReceiveFullConnectionCloseResponseWithoutErrorAtALowDataRate at 2019-09-09T09:05:06
| [0.068s] Microsoft.AspNetCore.Hosting.Diagnostics Debug: Hosting starting
| [0.071s] Microsoft.AspNetCore.Hosting.Diagnostics Debug: Hosting started
| [0.071s] Microsoft.AspNetCore.Hosting.Diagnostics Debug: Loaded hosting startup assembly Sockets.FunctionalTests, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60
| [0.072s] Microsoft.AspNetCore.Server.Kestrel Debug: TestServer is listening on port 34479
| [0.074s] Microsoft.AspNetCore.Server.Kestrel Debug: Connection id "0HLPL0J6HPQ7C" accepted.
| [0.077s] Microsoft.AspNetCore.Server.Kestrel Debug: Connection id "0HLPL0J6HPQ7C" started.
| [0.078s] Microsoft.AspNetCore.Hosting.Diagnostics Information: Request starting HTTP/1.1 GET http:/// - -
| [11.186s] Microsoft.AspNetCore.Hosting.Diagnostics Information: Request finished HTTP/1.1 GET http:/// - - - 200 - - 11107.8383ms
| [11.187s] Microsoft.AspNetCore.Server.Kestrel Debug: Connection id "0HLPL0J6HPQ7C" disconnecting.
| [11.188s] Microsoft.AspNetCore.Server.Kestrel Debug: Connection id "0HLPL0J6HPQ7C" stopped.
| [11.188s] Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets Debug: Connection id "0HLPL0J6HPQ7C" sending FIN because: "The Socket transport's send loop completed gracefully."
| [14.193s] Microsoft.AspNetCore.Hosting.Diagnostics Debug: Hosting shutdown
| [14.886s] TestLifetime Information: Finished test ClientCanReceiveFullConnectionCloseResponseWithoutErrorAtALowDataRate in 14.885012699999999s

This also could be an issue with how we are handling socket shutdown. Here is the shutdown code in Kestrel: https://github.com/aspnet/AspNetCore/blob/master/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs#L337

@tmds can you give me a TL;DR on what changed here in Socket.Shutdown + Socket.Dispose on Unix?

cc @halter73

[stephentoub]

Same question here. Is this possibly making things worse?

[davidfowl]

We currently have a 10% regression that we’re chasing down. So I’d wait on this if we’re. Worried it could regress even more

[tmds]

@stephentoub thank you for reviewing! I've addressed comments. For two open questions:

// On OSX, TCP connections will be closed with a FIN close instead of an abortive RST close.
// And, pending TCP connect operations and UDP receive are not abortable.

I ran tests of the PR without the other changes in #38804. The tests I'm most interested in (ConnectGetsCanceledByDispose, UdpReceiveGetsCanceledByDispose on macOS) show neither a pass/fail status: https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F39079~2Fmerge/test~2Ffunctional~2Fcli~2Finnerloop~2F/20190702.1/workItem/System.Net.Sockets.Tests. I'm not sure what the CI result means then, but it was not passing.

if (fdFlags == 0)
{
return false;
}

This is to avoid changing non-CLOEXEC sockets that may be shared with other Processes. I've augmented this to skip the check when the close is abortive (like when the Dispose happens from the finalizer).

[stephentoub]

I'm not sure what the CI result means then, but it was not passing

Actually I believe it was. For infrastructure reasons, passing data is at least temporarily not being stored. And if you look at the infrastructure log, you'll see the full xunit output, which shows:

===========================================================================================================
~/dotnetbuild/work/3be7edae-df7d-49bf-844e-5d3896a7ea71/Work/61c25c17-c2c2-4d21-a0c3-d09c26840d10/Exec ~/dotnetbuild/work/3be7edae-df7d-49bf-844e-5d3896a7ea71/Work/61c25c17-c2c2-4d21-a0c3-d09c26840d10/Exec
  Discovering: System.Net.Sockets.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Sockets.Tests (found 715 of 1089 test cases)
  Starting:    System.Net.Sockets.Tests (parallel test collections = on, max threads = 8)
    System.Net.Sockets.Tests.UnixDomainSocketTest.Socket_CreateUnixDomainSocket_Throws_OnWindows [SKIP]
      Condition(s) not met: "IsSubWindows10"
    System.Net.Sockets.Tests.KeepAliveTest.Socket_KeepAlive_RetryCount_Failure [SKIP]
      Condition(s) not met: "IsWindowsBelow1703"
    System.Net.Sockets.Tests.CreateSocket.Ctor_Raw_Supported_Success [SKIP]
      Condition(s) not met: "SupportsRawSockets"
    System.Net.Sockets.Tests.SendReceiveMemoryNativeTask.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveTask.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveApm.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveMemoryArrayTask.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveEap.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveMemoryNativeTask.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveTask.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveApm.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveEap.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveMemoryArrayTask.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionReset
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1106,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendFileTest.SyncSendFileGetsCanceledByDispose [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendFile.cs(230,0): at System.Net.Sockets.Tests.SendFileTest.SyncSendFileGetsCanceledByDispose()
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSpanSync.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1061,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.AcceptSyncForceNonBlocking.AcceptGetsCanceledByDispose [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/Accept.cs(320,0): at System.Net.Sockets.Tests.Accept`1.AcceptGetsCanceledByDispose()
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.AcceptSync.AcceptGetsCanceledByDispose [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/Accept.cs(320,0): at System.Net.Sockets.Tests.Accept`1.AcceptGetsCanceledByDispose()
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSyncForceNonBlocking.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1061,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSync.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1061,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSpanSyncForceNonBlocking.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: True) [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1061,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSpanSyncForceNonBlocking.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionAborted
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1117,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSync.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionAborted
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1117,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSyncForceNonBlocking.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionAborted
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1117,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
    System.Net.Sockets.Tests.SendReceiveSpanSync.TcpReceiveSendGetsCanceledByDispose(receiveOrSend: False) [FAIL]
      Assert.Equal() Failure
      Expected: ConnectionAborted
      Actual:   (null)
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(1117,0): at System.Net.Sockets.Tests.SendReceive`1.TcpReceiveSendGetsCanceledByDispose(Boolean receiveOrSend)
        --- End of stack trace from previous location where exception was thrown ---
  Finished:    System.Net.Sockets.Tests
=== TEST EXECUTION SUMMARY ===
   System.Net.Sockets.Tests  Total: 927, Errors: 0, Failed: 21, Skipped: 3, Time: 93.568s

[tmds]

Actually I believe it was. For infrastructure reasons, passing data is at least temporarily not being stored. And if you look at the infrastructure log, you'll see the full xunit output, which shows:

Thanks. The Connect and UdpReceive tests were skipped on macOS. I've included them now and another CI build is in progress: #39079.

[tmds]

No regression on macOS for {Connect,UdpReceive}GetsCanceledByDispose. Pre-PR:

    System.Net.Sockets.Tests.ConnectSync.ConnectGetsCanceledByDispose [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/Connect.cs(118,0): at System.Net.Sockets.Tests.Connect`1.ConnectGetsCanceledByDispose()

    System.Net.Sockets.Tests.SendReceiveSync.UdpReceiveGetsCanceledByDispose [FAIL]
      Assert.NotSame() Failure
      Stack Trace:
        /_/src/System.Net.Sockets/tests/FunctionalTests/SendReceive.cs(979,0): at System.Net.Sockets.Tests.SendReceive`1.UdpReceiveGetsCanceledByDispose()

[tmds]

@geoffkizer @davidfowl can you please look into this PR? A previous attempt was reverted after being merged just before preview 7. It would be good if this gets some more time on master.

[stephentoub]

Thanks, Tom. At this point I think it's going to miss 3.0, and we'll check it in first thing after master branches. The bar is high right now, and we just don't have any more runway to deal with fallout; while I really want this change in, it has the potential to be destabilizing, have unintended perf regressions, etc.