Avoid NullReferenceException in TcpClient.EndConnect

[Paciv]

When the timeout is shorter than the time TcpClient.ConnectAsync takes to throw
The task.Wait returns before the end of the task, then dispose the Tcpclient before the end of its ConnectAsync call

[scottf]

Could you possibly make a unit test to demonstrate this?

[Paciv]

It actually only throws a null reference exception internally on the task that is not awaited anymore, because the TcpClient have been disposed before the end of its ConnectAsync method.

I did a small repro case, but I don't see how that could fit in a unit test

        static void Main(string[] args)
        {
            TcpClient tcpClient = new();            
            var task = tcpClient.ConnectAsync("unreachable.destination.com", 4444);

            Exception nce = null;
            try
            {
                // force too short timeout
                if (!task.Wait(TimeSpan.FromMilliseconds(1)))
                {
                    nce = new Exception("timeout");
                }
            }
            catch (AggregateException ex)
            {
                nce = new Exception(ex.InnerException.Message);
            }

            if (nce != null)
            {
                tcpClient.Dispose();
                
                // Get the TcpClient NullReference exception (should get a UnknownHost instead)
                task.Wait();

                tcpClient = null;
                throw nce;
            }
        }

[scottf]

This I understand. The task is still running even though we aren't waiting for it. Since we timed-out we don't want the connection, so close it.

[scottf]

Isn't it possible that the same thing happens? Maybe a better question is when would an exception be thrown.

[Paciv]

If an exception is thrown here, it means that the task is completed, and that it ended in an exception.
That's why we can safely dispose the TcpClient

[scottf]

But if the task of making the tcp client failed, is there anything to safely close?

[Paciv]

It disposes the TcpClient and its underlying Socket in a timely manner, after the connection failed.
If not disposed here, it is bound to be disposed at some point by the GC because the reference is not rooted, but it is always better to avoid unpredictable behaviors.

[scottf]

Can we wrap it in an exception handler just so it doesn't fail. It's probably overkill but doesn't really hurt anything. Other than that this is ready to merge.

[Paciv]

It looks overkill, yes.
Moreover, I wouldn't know what to do with the new exception at this point. There is already one to throw, so the new one would be logged at best or hidden, which isn't really good design.
I believe that if an internal exception is thrown by the .NET Framework Dispose on the TcpClient, we may want to rethrow it anyway, because it would mean something very bad happened.

Also this hasn't been sufficiently tested to be released as is, there would be the need to test connectivity on a mix of multiple reachable and non-reachable node list.