Socket.ConnectAsync with an IPAddress and a failed connection leaves a Socket for finalization

[stephentoub]

The static Socket.ConnectAsync method creates one or more Sockets to use to establish the connection, and that Socket is then returned when the connection is made successfully. When the connection fails, it's supposed to Dispose of the created Socket(s). But it's failing to do so when something other than a DnsEndPoint is supplied, hitting this code path:
https://github.com/dotnet/corefx/blob/4fc38265fb35fd6bfc982b8e35a2545502e596b3/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L3975-L3976
that creates the Socket but then uses the public ConnectAsync(SocketAsyncEventArgs), which can't Dispose of the Socket because it doesn't know that it's one internally created (since it's a public method and the caller owns the Socket instance). This ends up leaking a socket handle / file descriptor until the Socket / SafeHandle's finalizer runs to close it, eventually.

This issue exists in both .NET Core and .NET Framework, and appears to have existed for a long time. However, it's showing up as an issue for SocketsHttpHandler, due to this optimization:
https://github.com/dotnet/corefx/blob/4fc38265fb35fd6bfc982b8e35a2545502e596b3/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs#L57-L59
where SocketsHttpHandler is trying to take the more optimized IPEndPoint path when an IP address is supplied. This then means that HttpClient failed connects end up leaking a handle / file descriptor similarly until the finalizer runs, which if lots of requests are being made, could result in "too many open files" errors on Unix.

I'm marking this as a 2.1 issue just for the SocketsHttpHandler fix, as it's a new issue in 2.1. The Sockets issue should be addressed, but it's long-standing and can be deferred to 2.2.

[Jodacola]

Following up on tests from the SignalR issue:

Thanks, Jodacola. Could you also try using "127.0.0.1" but adding GC.Collect(); GC.WaitForPendingFinalizers(); into your loop? Just for additional confirmation... I expect/hope you'll see the issue similarly go away.

Still leaking with the forced GC. Tests ran:

1. localhost without GC, no leaking (described at Performance leak aspnet/SignalR#2124 (comment))
2. With 127.0.0.1 without GC (to verify after previous test), leaking
3. With 127.0.0.1 with GC, leaking
4. With 127.0.0.1 with GC (second test with full clean and rebuild), leaking
5. Final test with localhost without GC for confirmation, no leaking.

Silly test code (comment/uncomment accordingly):

class Program
{
    static HttpClient client;

    static void Main(string[] args)
    {
        Task.Run(ConnectClient);
        System.Threading.Thread.Sleep(Int32.MaxValue);
    }

    public static async Task ConnectClient()
    {
        client = new HttpClient();
        //var url = "http://127.0.0.1:5000/badhub/";
        var url = "http://localhost/badhub/";
        var urlBuilder = new UriBuilder(url);
        urlBuilder.Path += "negotiate";
        HttpResponseMessage response = null;
        while (true)
        {
            try
            {
                var req = new HttpRequestMessage(HttpMethod.Post, urlBuilder.Uri);
                response = await client.SendAsync(req);
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
                await Task.Delay(0); // Set delay low to invoke leak faster
            }
            //GC.Collect();
            //GC.WaitForPendingFinalizers();
        }
    }
}

[stephentoub]

Thanks, @Jodacola. re: GC, I'm very surprised by that. I suspect some locals are having their lifetime extended and preventing the GC.Collect from having the desired affect. Is your test debug or release?

[stephentoub]

@Jodacola, would you be able to try out the fix in dotnet/corefx#29286 and confirm whether it does or does not address the leak for you?

[Jodacola]

Haven't yet had an opportunity. Day job firefighting. 😄

Will try as soon as I can. Also, was running debug for the aforementioned tests.

[Jodacola]

I just finished running several tests with CoreFX built from master (since I saw your PR had been merged) and I'm no longer able to reproduce the socket leaks. Nice job!

[stephentoub]

Thanks for verifying!

[stephentoub]

The 2.1 part of this was merged. Moving to 2.2.