Socket considered connected when Select is interrupted by shutdown

[drieseng]

When a (blocking) Socket.Select(...) is interrupted by a Socket.Shutdown(SocketShutdown.Both) from another thread, in some cases Socket.Connected continues to return true for some time on Linux.

Even though Socket compatibility with .NET Framework has greatly improved in .NET 5.0 Preview 2 (due to dotnet/corefx#38804?), the result on Linux is still not always the same as we get on Windows.

On Windows (both .NET Core and .NET Framework), Socket.Connected immediately return false if Socket.Select(...) is interrupted because the socket is shut down.

To reproduce, compile and run the following code while specifying a number of iterations to run (I use 300 as value):

using System;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;

class SocketSelect
{
    private static int LastId = 0;

    static void Main(string[] args)
    {
        if (args.Length != 1)
        {
            Console.Error.WriteLine("Please specify the number of iterations.");
            Console.Error.WriteLine("For example:");
            Console.Error.WriteLine("dotnet SocketSelect.dll 300");
            return;
        }

        if (!int.TryParse(args[0], out var iterationCount))
        {
            Console.Error.WriteLine("Please specify a valid number of iterations.");
            Console.Error.WriteLine("For example:");
            Console.Error.WriteLine("dotnet SocketSelect.dll 300");
            return;
        }

        var endPoint = new IPEndPoint(IPAddress.Loopback, 8080);

        var serverTask = Task.Factory.StartNew((ep) =>
        {
            var listener = new TcpListener((IPEndPoint)ep);
            listener.Start();

            while (true)
            {
                var s = listener.AcceptSocket();

                while (true)
                {
                    var bytesReceived = s.Receive(new byte[1]);
                    if (bytesReceived == 0)
                    {
                        s.Shutdown(SocketShutdown.Both);
                        break;
                    }
                    s.Send(new byte[1]);
                }

                s.Close();
            }
        }, endPoint);

        Thread.Sleep(500);

        var threadId = Interlocked.Increment(ref LastId);

        for (var i = 0; i < iterationCount; i++)
        {
            var client = new Socket(endPoint.AddressFamily, SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
            client.Connect(endPoint);

            var allDataReceived = new ManualResetEvent(false);

            var listenerTask = Task.Factory.StartNew(s =>
            {
                var clientState = (ClientState)s;

                var socket = clientState.Socket;
                int dataReceivedCounter = 0;

                while (socket.Connected)
                {
                    try
                    {

                        var readSockets = new System.Collections.Generic.List<Socket> { socket };
                        Socket.Select(readSockets, null, null, -1);

                        if (dataReceivedCounter == 10)
                        {
                            // when we've received 10 "payloads", the socket should be shutdown
                            if (!socket.Connected)
                            {
                                break;
                            }

                            Console.WriteLine($"[{clientState.Id}] Connected | Data received counter = {dataReceivedCounter})");
                        }
                        else if (!socket.Connected)
                        {
                                break;
                        }
                    } catch (Exception ex)
                    {
                        Console.WriteLine($"[{clientState.Id}] Exception on Select ({dataReceivedCounter}): " + ex);
                        break;
                    }

                    try
                    {
                        var bytesReceived = socket.Receive(new byte[1]);
                        dataReceivedCounter++;

                        if (dataReceivedCounter == 10)
                        {
                            allDataReceived.Set();
                        }
                    }
                    catch (Exception ex)
                    {
                        Console.WriteLine($"[{clientState.Id}] Exception on Receive ({dataReceivedCounter}): " + ex);
                    }
                }
            }, new ClientState(client, threadId), TaskCreationOptions.LongRunning);

            for (var z = 0; z < 10; z++)
            {
                client.Send(new byte[1]);
            }

            allDataReceived.WaitOne();

            // Allow time for the message loop to enter the blocking Socket.Select(...) call
            Thread.Sleep(100);

            client.Shutdown(SocketShutdown.Both);

            if (!listenerTask.Wait(5000))
            {
                Console.WriteLine($"[{threadId}] Socket did not break out of Select call within 5 seconds.");
            }

            client.Dispose();

        }
    }

    private class ClientState
    {
        public ClientState(Socket socket, int id)
        {
            Socket = socket;
            Id = id;
        }

        public Socket Socket { get; }
        public int Id { get; }
    }
}

Expected result:
The application completes without any output produced.

Actual result (on Linux):
The following is output several times:

[1] Connected | Data received counter = 10)

Tagging @dotnet/ncl as an area owner. If you would like to be tagged for a label, please notify danmosemsft.

[wfurt]

cc: @tmds

[karelz]

Triage: Would be nice to have consistency across platforms.

[tmds]

Simplifying the repro: this code occasionally outputs Connected: true.

using System.Collections.Generic;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;

class SocketSelect
{
    static void Main(string[] args)
    {
        while (true)
        {
            (Socket socket1, Socket socket2) = CreateTcpPair();
            using var _1 = socket1;
            using var _2 = socket2;

            Task.Factory.StartNew(() =>
            {
                socket1.Shutdown(SocketShutdown.Both);
            });

            var readSockets = new List<Socket> { socket1 };
            Socket.Select(readSockets, null, null, -1);
            System.Console.WriteLine($"Connected : {socket1.Connected}");
        }
    }

    static (Socket socket1, Socket socket2) CreateTcpPair()
    {
        using var serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
        serverSocket.Bind(new IPEndPoint(IPAddress.Loopback, 0));
        serverSocket.Listen(1);
        var clientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
        clientSocket.Connect(serverSocket.LocalEndPoint);
        var acceptSocket = serverSocket.Accept();
        return (acceptSocket, clientSocket);
    }
}

Looking at the implementation of Socket.Shutdown:

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

Lines 3791 to 3813 in 6a8fd0b

	// Disables sends and receives on a socket.
	public void Shutdown(SocketShutdown how)
	{
	if (NetEventSource.IsEnabled) NetEventSource.Enter(this, how);
	ThrowIfDisposed();
	if (NetEventSource.IsEnabled) NetEventSource.Info(this, $"how:{how}");
	// This can throw ObjectDisposedException.
	SocketError errorCode = SocketPal.Shutdown(_handle, _isConnected, _isDisconnected, how);
	if (NetEventSource.IsEnabled) NetEventSource.Info(this, $"Shutdown returns errorCode:{errorCode}");
	// Skip good cases: success, socket already closed.
	if (errorCode != SocketError.Success && errorCode != SocketError.NotSocket)
	{
	UpdateStatusAfterSocketErrorAndThrowException(errorCode);
	}
	SetToDisconnected();
	InternalSetBlocking(_willBlockInternal);
	if (NetEventSource.IsEnabled) NetEventSource.Exit(this);
	}

In this code SetToDisconnected is called at some pointer after SocketPal.Shutdown. During this time, Select may return and the result will be Connected : True. Socket.Connected also doesn't have any synchronization to read the value that got updated from the other thread.

This code is the same for Windows and Linux.

When it happens, it is not a major issue since the next call made against the Socket will fail.

@karelz this behaves as expected, so I think we can close the issue.

@drieseng if you care about having this synchronization, you can use Interlocked operations to write a field before calling Socket.Shutdown and then check that field when Select returns, something like:

            int hasShutdown = 0;

            Task.Factory.StartNew(() =>
            {
                Interlocked.Exchange(ref hasShutdown, 1);
                socket1.Shutdown(SocketShutdown.Both);
            });

            var readSockets = new List<Socket> { socket1 };
            Socket.Select(readSockets, null, null, -1);
            System.Console.WriteLine($"Shutdown: {Volatile.Read(ref hasShutdown) != 0}");

[wfurt]

I'm wondering if it is worth of mentioning it in documentation. (or least mention the threading race condition) May not matter if the Window is really small.

[tmds]

Socket.Connected docs could mention the property is not thread-safe.

[tmds]

Docs updated in dotnet/dotnet-api-docs#4493. This may be closed.