TcpListener regression: SocketException at InputShare app

[jiangzeng01]

Application Name: Input Share
OS: Windows 10 RS5
CPU: X64
.NET Build Number: 5.0.100-rc.1.20430.2

Verify Scenarios:
1). Windows 10 RS5 X64 + .NET Core SDK build 5.0.100-rc.1.20430.2: Fail
2). Windows 10 RS5 X64 + .NET Core SDK build 5.0.100-preview.8 : Pass
3). Windows 10 RS5 X64 + .NET Core SDK build 3.1.300 Pass

DevDiv bug:
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1188270
Minimal Repro App:
https://github.com/jiangzeng01/socketexceptionrepro.git

Findings :
When we stop TcpListener, it BeingAccesSocket call back, it gets different exception in net5 rc1, it makes the app crash :
We start listener like this :

private static TcpListener listener;
listener = new TcpListener(IPAddress.Any, 4441);
listener.Start(6);
listener.BeginAcceptSocket(Listener_AcceptSocketCallback, null);
label1.Text = "started listening";

We stop like this :

listener.Stop();

In callBack function (Listener_AcceptSocketCallback) : 

private static void Listener_AcceptSocketCallback(IAsyncResult ar)
{
try
{
Socket client = listener.EndAcceptSocket(ar);
}
catch (ObjectDisposedException ex)
{
MessageBox.Show("ObjectDisposedException on 3.1 and 5 prev");
//caught on 3.1 and 5 prev, doesn't catch on .net5 rc1
}
catch (Exception ex)
{
 //since we stop it, we think ObjectDisposedException should be caught, but it comes to here in   //.net5 Rc1
  MessageBox.Show($"Catch on .net5 RC {ex.Message}");
 	listener.BeginAcceptSocket(Listener_AcceptSocketCallback, null);
}
}

Exception from InputShare app:

ISClientListener-> An error occurred while accepting client socket: The I/O operation has been aborted because of either a thread exit or an application request.
--------------------------------------
UNHANDLED EXCEPTION
Not listening. You must call the Start() method before calling this method.
   at System.Net.Sockets.TcpListener.AcceptSocketAsync()
   at System.Net.Sockets.TcpListener.BeginAcceptSocket(AsyncCallback callback, Object state)
   at InputshareLib.Net.ISClientListener.Listener_AcceptSocketCallback(IAsyncResult ar) in C:\Users\Appcompat\Documents\inputsharesource\Inputshare\InputshareLib\Net\ISClientListener.cs:line 72
   at System.Threading.Tasks.TaskToApm.TaskAsyncResult.InvokeCallback()
   at System.Threading.Tasks.AwaitTaskContinuation.<>c.<.cctor>b__17_0(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.AwaitTaskContinuation.RunCallback(ContextCallback callback, Object state, Task& currentTask)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__140_1(Object state)
   at System.Threading.QueueUserWorkItemCallbackDefaultContext.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
System.Net.Sockets

CC @dotnet-actwx-bot

[ghost]

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

[antonfirsov]

TcpClient propagates the exception from Socket, where the related Accept behavior has been changed in #40476.

There is a big inconsistency between the various code paths (Sync, APM, EAP/Task) when disposing a socket during accept. Only the APM path seems to throw ObjectDisposedException:

EAP/Task	System.Net.Sockets.SocketException	The I/O operation has been aborted because of either a thread exit or an application request. (995)
Sync	System.Net.Sockets.SocketException	A blocking operation was interrupted by a call to WSACancelBlockingCall. (10004)
APM	System.ObjectDisposedException	Cannot access a disposed object.

Repro: add the following test to Accept.cs:

[Fact]
public async Task AcceptAsync_WhenDisposed_ThrowsObjectDisposedException()
{
    Socket listen = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    int port = listen.BindToAnonymousPort(IPAddress.Loopback);
    listen.Listen(1);

    using Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    
    Task<Socket> acceptTask = AcceptAsync(listen);

    // give some time for the OS calls:
    await Task.Delay(100);
    listen.Dispose();
    _ = client.ConnectAsync(new IPEndPoint(IPAddress.Loopback, port));

    Exception ex = await Assert.ThrowsAnyAsync<Exception>(() => acceptTask);

    string socketErrorCode = ex is SocketException sx ? $"({sx.ErrorCode})" : "";
    _output.WriteLine($"{ex.GetType()} | {ex.Message} {socketErrorCode}");
}

@geoffkizer @scalablecory should we synchronize the behavior for 5.0? If yes, should we map to ObjectDisposedException in all those cases?

[scalablecory]

It's expected for Task-based async to throw with SocketError.OperationAborted; we shouldn't change that. Actually I'm surprised that APM doesn't. Are you sure you aren't seeing a pre-disposed socket VS an I/O in progress and then a dispose?

Do we have a regression from 3.x or is this just things being different?

[antonfirsov]

Are you sure you aren't seeing a pre-disposed socket VS an I/O in progress and then a dispose?

@scalablecory if my understanding is correct, with APM there is no way / no point to distinguish those cases from API users perspective. The exception is always being thrown when EndAccept(IAR) gets called. This is how it is implemented currently:

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

Lines 3549 to 3555 in 2ba6515

	public Socket EndAccept(IAsyncResult asyncResult)
	{
	return EndAcceptCommon(out _, out _, asyncResult);
	}
	private Socket EndAcceptCommon(out byte[]? buffer, out int bytesTransferred, IAsyncResult asyncResult)
	{
	ThrowIfDisposed();

From the perspective of how APM API-s work, it seems actually logical to throw ObjectDisposedException here.

Since we delegate the exception throwing logic from TcpListener to Socket now, this exception is no longer thrown since BeginAccept / EndAccept utilizes the EAP Socket path now in TcpListener. This is an unexprected breaking change (="regression") in 5.0.

Maybe I'm wrong here and exceptions should not be consistent, but we need a decision ASAP. If we can't clarify this, maybe we should just revert #40476 to prevent the breaking change. The inconsistency between the implementation (Socket VS TcpListener) is probably not a very good state for the product code anyways.

[geoffkizer]

Making this work like 3.1 is easy enough, we should just add the ThrowIfDisposed check to EndAccept.

I agree that it would be nice to clean up our exceptions to be more consistent here -- we should look at that for 6.0. But right now let's just address this particular issue.

[geoffkizer]

What's particularly weird about this is that TcpListener does not actually implement IDisposable, yet we throw ObjectDisposedException anyway. Strange.

[jiangzeng01]

@antonfirsov @karelz Just want to confirm will the fix go into RC1 branch? I saw you have mentioned in the PR that the fix will not included in RC1 and document it as known issue.

[antonfirsov]

@jiangzeng01 unfortunately the change will land in RC2 only.

[karelz]

@jiangzeng01 that is correct, this should be only documented as a known issue for RC1.

[karelz]

Fixed in 6.0 (PR #41745) and in 5.0 RC2 (PR #41938) ... 5.0 RC1 will be only documented as known issue.