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]

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.