Modified PR for reverse diagnostics server

[josalem]

This PR un-reverts the reverse feature of the Diagnostics Server and adds fixes for a HANDLE-use-after-close issue that cropped up in CI (see #35451).

I recommend looking at this PR commit by commit, to filter out the revert changes (776724f) from the fixes introduced in this PR.

I'll open this PR in draft mode to run some outer loop CI over it and then open it for further review.

Details of changes:

• Add a mechanism for canceling out of IpcStreamFactory::Poll
  • should prevent regression for Diagnostics Server IPC thread causes 20-40ms shutdown delays on Windows #12991
• Remove attempts to do too much cleanup on process shutdown, e.g., don't attempt to close handles or cancel overlapped IO.
  • should fix issue found in Test hang in System.Diagnostics.Tracing.EventProvider.EventUnregister #35451

CC - @tommcdon @noahfalk @jkotas @sywhang

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon
Notify danmosemsft if you want to be subscribed.

[josalem]

/azp list

[azure-pipelines]

CI/CD Pipelines for this repository:

• coreclr-gc-longrunning
• coreclr-gc-simulator
• runtime-coreclr outerloop
• runtime-coreclr runincontext
• runtime-coreclr crossgen2
• runtime-libraries outerloop
• runtime-libraries outerloop-windows
• runtime-libraries outerloop-linux
• runtime-libraries outerloop-osx
• runtime
• runtime-libraries enterprise-linux
• runtime-libraries stress-http
• runtime-libraries stress-ssl
• runtime-live-build
• runtime-coreclr crossgen2 outerloop
• coreclr-release-outerloop-nightly
• sync-runtime-to-mono
• dotnet-runtime-perf
• runtime-coreclr crossgen2-composite
• runtime-jit-experimental

[josalem]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[josalem]

Failed leg's test failure looks to have been hit before in #32955. Rerunning...

[josalem]

Socket test failures look to be existing with failures reported as recent as yesterday. It repros on my Mac for all currently shipped previews of 5.0, albeit with a slightly different failure:

> runtime/artifacts/bin/testhost/netcoreapp5.0-OSX-Release-x64/dotnet exec --runtimeconfig System.Net.Sockets.Tests.runtimeconfig.json --depsfile System.Net.Sockets.Tests.deps.json xunit.console.dll System.Net.Sockets.Tests.dll -xml testResults.xml -nologo -method System.Net.Sockets.Tests.TcpClientTest.Dispose_CancelsConnectAsync -notrait category=failing
  Discovering: System.Net.Sockets.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Sockets.Tests (found 1 of 1201 test case)
  Starting:    System.Net.Sockets.Tests (parallel test collections = on, max threads = 8)
    System.Net.Sockets.Tests.TcpClientTest.Dispose_CancelsConnectAsync(connectByName: True) [FAIL]
      System.Net.Sockets.SocketException : Unknown error
      Stack Trace:
        runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs(1138,0): at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
        runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs(1123,0): at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
        runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs(220,0): at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
        --- End of stack trace from previous location ---
        runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs(320,0): at System.Net.Sockets.TcpClient.CompleteConnectAsync(Task task)
        runtime/src/libraries/System.Net.Sockets/tests/FunctionalTests/TcpClientTest.cs(435,0): at System.Net.Sockets.Tests.TcpClientTest.Dispose_CancelsConnectAsync(Boolean connectByName)
        --- End of stack trace from previous location ---
  Finished:    System.Net.Sockets.Tests
=== TEST EXECUTION SUMMARY ===
   System.Net.Sockets.Tests  Total: 2, Errors: 0, Failed: 1, Skipped: 0, Time: 0.253s

Edit: Has an issue tracking it now -> #35886

Linq test failures also appear to be existing and have been failing libraries-outerloop since at least Last Friday 4/15.

The TestOnStartWithArgsThenStop failure appears to be a reoccurrence of #34801.

I'm not convinced that any of these failures are due to this change, but will do some due diligence to be more confident in that assertion.

[josalem]

Ran some benchmarking (avg, max, min for 1000 measurements) for the scenario in #12991 and got the following results:

3.1

$ C:\git\scratch\benchmark.ps1 { ./ConsoleApp.exe } -Samples 1000 -Silent
Average: 70.7435069ms
Minimum: 64.9551ms
Maximum: 87.1662ms

5.0-preview3

$ C:\git\scratch\benchmark.ps1 { ./ConsoleApp.exe } -Samples 1000 -Silent
Average: 62.6742732000001ms
Minimum: 56.4682ms
Maximum: 98.195ms

This PR's coreclr.dll built on top of master in a self-contained publish of preview 3:

$ C:\git\scratch\benchmark.ps1 { ./ConsoleApp.exe } -Samples 1000 -Silent
Average: 65.2096325ms
Minimum: 59.7252ms
Maximum: 85.6353ms

so we have between preview 3 and this PR on top of master (AKA preview 5).

If I recall, the original issue for #12991 only showed when the app was executed on a machine with Hyper-V turned off. I'm not sure if I can turn off Hyper-V on my work desktop since I'm remoting into it from home and can't touch the UEFI options, but I'll try to run these same experiments on a Windows box with Hyper-V off.

[jkotas]

the original issue for #12991 only showed when the app was executed on a machine with Hyper-V turned off

That sounds suspect. Did we get to a root cause for #12991 ? I am wondering whether it could have been another shutdown race in eventpipe that - depending on timing - destroyed something that was still in use. Hyper-V turned off just changed the timing to trigger the problem.

[josalem]

I'm trying to find explicit documentation of the issue repro steps. In the meantime, this was the code added to fix the issue: https://github.com/dotnet/coreclr/pull/25602/files#diff-776272f612a5b488defb167f8afb2333R212-R232

According to the associated PR, the root cause was a difference in time between the OS unblocking the ConnectNamedPipe call + collecting the server thread and manually canceling the ConnectNamedPipe call to allow the Server thread to run to completion.

[jkotas]

https://github.com/dotnet/coreclr/pull/25602/files#diff-776272f612a5b488defb167f8afb2333R212-R232

This code was deleted in dotnet/coreclr#27136 because of it was again causing intermittent hangs during shutdown.

[josalem]

Ah, found a more detailed description/discussion: #13563

In light of the discussion in that issue and the fact that 3.1 doesn't have this code path (it only attempts to unlink the unix domain socket on linux) I'm starting to think I should discount #12991 until I can find explicit documentation that this issue is applicable. Thoughts?

@sergiy-k I believe the original report for that issue came through you from a partner team. Do you have any further information on this floating around your inbox?

[josalem]

Talked with Sergiy offline, and it sounds like the original issue was specifically on x64 with "virtualization support" turned off at the BIOS/UEFI level. Additionally, the issue wasn't consistent in nature and was more that there was a variation in the execution time that sometimes had this delay. I'm working on getting a machine with virtualization support turned off to run these benchmarks on and see if this behavior is still present.

[josalem]

Worked offline with @noahfalk to test runtimes on a machine with "Virtualization Support" turned off. We couldn't reproduce any slowdown effects. Furthermore, we concluded that there wasn't an apparent slowdown from removing the cleanup logic. I've modified the PR to only perform minimal cleanup for Diagnostics Server resources on shutdown, i.e., it will only unlink the Unix Domain Socket on non-Windows platforms.

[josalem]

CI failure looks to be #35877. I'm going to rebase and rerun CI.

[josalem]

@msftbot merge after @noahfalk approves

[noahfalk]

LGTM