Closing the AsyncWaitHandle from the result of socket.BeginConnect crashes the application in .NET 6.0

[MaxG117]

Description

The simple code below works on all versions of .NET before 6.0, but crashes with "Unhandled exception. System.ObjectDisposedException: Safe handle has been closed." on net6.0:

using System;
using System.Net;
using System.Net.Sockets;

namespace Net60SafeHandleBug
{
    class Program
    {
        static void Main(string[] args)
        {
            int port = 1234;
            IPAddress.TryParse("127.0.0.1", out IPAddress ipAddress);
            Socket socket = new Socket(ipAddress.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
            IAsyncResult asyncResult = socket.BeginConnect(ipAddress, port, null, null);
            // do something, then suddenly quit...
            socket.Close();
            asyncResult.AsyncWaitHandle.Close(); // <-- ruh roh

            Console.WriteLine("Press any key to quit...");
            Console.ReadKey();
        }
    }
}

Reproduction Steps

Build the above code as a console app. It will crash when targeting net6.0, but work fine with any older version of .NET.

Expected behavior

The application should succeed. The documentation for IAsyncResult.AsyncWaitHandle does not mention that one must wait for the handle to be signaled or call the APM End* method before closing, but it does say that the handle should be closed when the application is done using it.

Actual behavior

The application crashes with the following:

System.ObjectDisposedException
  HResult=0x80131622
  Message=Safe handle has been closed.
Object name: 'SafeHandle'.
  Source=System.Private.CoreLib
  StackTrace:
   at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success)
   at Interop.Kernel32.SetEvent(SafeWaitHandle handle)
   at System.Threading.EventWaitHandle.Set()
   at System.Threading.ManualResetEventSlim.Set(Boolean duringCancellation)
   at System.Threading.Tasks.Task.FinishStageTwo()
   at System.Threading.Tasks.Task.FinishSlow(Boolean userDelegateExecute)
   at System.Threading.Tasks.Task.TrySetException(Object exceptionObject)
   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.InvokeContinuation(Action`1 continuation, Object state, Boolean forceAsync, Boolean requiresExecutionContextFlow)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.OnCompleted(SocketAsyncEventArgs _)
   at System.Net.Sockets.SocketAsyncEventArgs.OnCompletedInternal()
   at System.Net.Sockets.SocketAsyncEventArgs.HandleCompletionPortCallbackError(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Net.Sockets.SocketAsyncEventArgs.<>c.<.cctor>b__179_0(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading.ThreadPoolBoundHandleOverlapped.CompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pNativeOverlapped)

Regression?

This is a regression in .NET 6.0, because it works in all the prior versions.

Known Workarounds

No response

Configuration

Tested on net461, netcoreapp2.0, net5.0, and net6.0 running on Windows 10 (x64). Did not test on other platforms.

Other information

No response

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The simple code below works on all versions of .NET before 6.0, but crashes with "Unhandled exception. System.ObjectDisposedException: Safe handle has been closed." on net6.0:

using System;
using System.Net;
using System.Net.Sockets;

namespace Net60SafeHandleBug
{
    class Program
    {
        static void Main(string[] args)
        {
            int port = 1234;
            IPAddress.TryParse("127.0.0.1", out IPAddress ipAddress);
            Socket socket = new Socket(ipAddress.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
            IAsyncResult asyncResult = socket.BeginConnect(ipAddress, port, null, null);
            // do something, then suddenly quit...
            socket.Close();
            asyncResult.AsyncWaitHandle.Close(); // <-- ruh roh

            Console.WriteLine("Press any key to quit...");
            Console.ReadKey();
        }
    }
}

Reproduction Steps

Build the above code as a console app. It will crash when targeting net6.0, but work fine with any older version of .NET.

Expected behavior

The application should succeed. The documentation for IAsyncResult.AsyncWaitHandle does not mention that one must wait for the handle to be signaled or call the APM End* method before closing, but it does say that the handle should be closed when the application is done using it.

Actual behavior

The application crashes with the following:

System.ObjectDisposedException
  HResult=0x80131622
  Message=Safe handle has been closed.
Object name: 'SafeHandle'.
  Source=System.Private.CoreLib
  StackTrace:
   at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success)
   at Interop.Kernel32.SetEvent(SafeWaitHandle handle)
   at System.Threading.EventWaitHandle.Set()
   at System.Threading.ManualResetEventSlim.Set(Boolean duringCancellation)
   at System.Threading.Tasks.Task.FinishStageTwo()
   at System.Threading.Tasks.Task.FinishSlow(Boolean userDelegateExecute)
   at System.Threading.Tasks.Task.TrySetException(Object exceptionObject)
   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.InvokeContinuation(Action`1 continuation, Object state, Boolean forceAsync, Boolean requiresExecutionContextFlow)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.OnCompleted(SocketAsyncEventArgs _)
   at System.Net.Sockets.SocketAsyncEventArgs.OnCompletedInternal()
   at System.Net.Sockets.SocketAsyncEventArgs.HandleCompletionPortCallbackError(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Net.Sockets.SocketAsyncEventArgs.<>c.<.cctor>b__179_0(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading.ThreadPoolBoundHandleOverlapped.CompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pNativeOverlapped)

Regression?

This is a regression in .NET 6.0, because it works in all the prior versions.

Known Workarounds

No response

Configuration

Tested on net461, netcoreapp2.0, net5.0, and net6.0 running on Windows 10 (x64). Did not test on other platforms.

Other information

No response

Author:	MaxG117
Assignees:	-
Labels:	area-System.Net, untriaged
Milestone:	-

[stephentoub]

The documentation for IAsyncResult.AsyncWaitHandle does not mention that one must wait for the handle to be signaled or call the APM End* method before closing

It may not say so (though it does say "Once created, AsyncWaitHandle should be kept alive until the user calls the method that concludes the asynchronous operation. At that time the object behind AsyncWaitHandle can be discarded."), but that is the intention. Until the operation has quiesced, it's going to want to set that wait handle when it completes, and if you close it out from under it, there's inherently a race condition between you closing it and the implementation trying to use it.

[MaxG117]

We believe that Socket.Close() is supposed to cancel or abort any outstanding APM operation: "The Close method closes the remote host connection and releases all managed and unmanaged resources associated with the Socket." We shouldn't have to wait for the outstanding socket operations to complete, and since there's no other way to cancel the BeginConnect, Socket.Close() is supposed to do that, and this is how it has worked since the early days of .NET Framework. After Socket.Close is called, there should be no operation attempting to set the wait handle, so we should be able to close the handle.

[MaxG117]

@stephentoub:

I ran some experiments. In .NET 5.0 and earlier, the wait handle is always signaled by the time socket.Close() returns. In .NET 6.0, socket.Close() returns much faster (~0.2ms vs 3ms) and the wait handle is not signaled until later.

The paragraph you quoted is in the "Notes to Implementers" section. I.e., the implementers of IAsyncResult. I think you implied that it is my responsibility to wait for the wait handle, while the actual document states that it is the responsibility of the implementer of IAsyncResult, which in this case is Socket. Here's the entire paragraph (emphasis mine):

The object that implements IAsyncResult does not need to create the WaitHandle until the AsyncWaitHandle property is read. It is the choice of the IAsyncResult implementer. However, if the implementer creates AsyncWaitHandle, it is the responsibility of the implementer to signal the WaitHandle that will terminate the wait at the appropriate time. For example, AsyncResult terminates the wait on behalf of the caller when an asynchronously invoked method returns. Once created, AsyncWaitHandle should be kept alive until the user calls the method that concludes the asynchronous operation. At that time the object behind AsyncWaitHandle can be discarded.

Further, in the "Notes to Callers" it states:

Note: When you use the BeginInvoke method of a delegate to call a method asynchronously and obtain a wait handle from the resulting IAsyncResult, we recommend that you close the wait handle as soon as you are finished using it, by calling the Close() method. If you simply release all references to the wait handle, system resources are freed when garbage collection reclaims the wait handle, but garbage collection works more efficiently when disposable objects are explicitly closed or disposed. For more information, see the AsyncWaitHandle property.

The .NET Socket implementation is the implementer of the IAsyncResult interface. My application is the user of that implementation. I called the socket.Close() method to conclude the asynchronous operation, since there's no other method to conclude or cancel the socket.BeginConnect() operation. Therefore I should now be able to discard the object behind AsyncWaitHandle by calling Close(), as described in the documentation.

[stephentoub]

while the actual document states that it is the responsibility of the implementer of IAsyncResult

That's intended to mean the disposal should be done as part of the EndXx call, not that the implementor waits on their own event. In fact most implementations don't even create the event until someone asks for it.

Further, in the "Notes to Callers" it states:

That's specifically about Delegate.BeginInvoke. It's not relevant. It's not even functional anymore.

[MaxG117]

@stephentoub I see you targeted 7.0 -- is there any chance this can be fixed in 6.0? 6.0 is the LTS that'll be around for many years. Thank you!

[GSPP]

Who is actually responsible for closing a wait handle that an IAsyncResult produced? I always assumed that's the responsibility of the implementer (typically done in the end method). A user should never close that handle.

This piece of documentation seems to contradict that (so I think it's wrong):

we recommend that you close the wait handle as soon as you are finished using it, by calling the Close() method

If that documentation is wrong, @MaxG117 could simply stop closing the handle.

@MaxG117 If you are looking for an immediate workaround, you can create your own handle or use the Task based API and block on that task (I'd prefer to do that over the APM pattern in any case).

[stephentoub]

see you targeted 7.0 -- is there any chance this can be fixed in 6.0?

Any chance, sure :-) but at this point I expect we'd want to see more devs reporting an issue before we took a servicing fix for it. This Task code has been like this for over a decade, and while Task wasn't used as the IAsyncResult for BeginConnect until relatively recently, it has been used without any such reported issues as the IAsyncResult for most APM implementations in NET Core for years, including for most other Socket APM implementations.

As noted, it's really not recommended to Dispose the wait handle yourself, in particular not before the operation completes (for better or worse the docs show closing it after completion, which is more reasonable, as at that point you can be more sure no one else is still using it, and if EndXx did Dispose it, your disposal will just nop). It's typically owned by the APM implementation, which may dispose it as part of EndXx, and it can otherwise be left for finalization (while we generally want to avoid finalization for perf reasons, if you're using the APM pattern and synchronously blocking on the wait handle, perf is not a primary concern). On top of that, the APM pattern is largely defunct and definitely not recommended for new code; existing code should be migrating off of it if at all possible. And even if code continues using it, the sync blocking on a wait handle for the IAsyncResult is also not the preferred approach, as such blocking leads to poorer scalability and greater risk of starvation and deadlocks. Most of our custom APM implementations have been ripped out and replaced by just layering on top of the corresponding Task-based APIs, which are easier to consume and better performing.

it doesn't help that the APM pattern is really hard to both implement and consume, which also means there have historically been many inconsistencies in implementations, many subtle bugs in code consuming it, and less than stellar docs around it.