Fix Timer rooting issue with SocketsHttpHandler (dotnet/corefx#32793)

dotnet/corefx#32793

Author: TalAloni

StandardSocketsHttpHandler.FunctionalTests/SocketsHttpHandlerTest.cs

@@ -9,6 +9,7 @@
 using System.Net.Security;
 using System.Net.Sockets;
 using System.Net.Test.Common;
+using System.Runtime.CompilerServices;
 using System.Security.Authentication;
 using System.Security.Cryptography.X509Certificates;
 using System.Text;
@@ -940,6 +941,48 @@ await server.AcceptConnectionAsync(async connection2 =>
                 }
             }
         }
+
+        [OuterLoop]
+        [Fact]
+        public void HandlerDroppedWithoutDisposal_NotKeptAlive()
+        {
+            var tcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+            HandlerDroppedWithoutDisposal_NotKeptAliveCore(tcs);
+            for (int i = 0; i < 10; i++)
+            {
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+            }
+            Assert.True(tcs.Task.IsCompleted);
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private void HandlerDroppedWithoutDisposal_NotKeptAliveCore(TaskCompletionSource<bool> setOnFinalized)
+        {
+            // This relies on knowing that in order for the connection pool to operate, it needs
+            // to maintain a reference to the supplied IWebProxy.  As such, we provide a proxy
+            // that when finalized will set our event, so that we can determine the state associated
+            // with a handler has gone away.
+            IWebProxy p = new PassthroughProxyWithFinalizerCallback(() => setOnFinalized.TrySetResult(true));
+
+            // Make a bunch of requests and drop the associated HttpClient instances after making them, without disposal.
+            Task.WaitAll((from i in Enumerable.Range(0, 10)
+                          select LoopbackServer.CreateClientAndServerAsync(
+                              url => new HttpClient(new StandardSocketsHttpHandler { Proxy = p }).GetStringAsync(url),
+                              server => server.AcceptConnectionSendResponseAndCloseAsync())).ToArray());
+        }
+
+        private sealed class PassthroughProxyWithFinalizerCallback : IWebProxy
+        {
+            private readonly Action _callback;
+
+            public PassthroughProxyWithFinalizerCallback(Action callback) => _callback = callback;
+            ~PassthroughProxyWithFinalizerCallback() => _callback();
+
+            public ICredentials Credentials { get; set; }
+            public Uri GetProxy(Uri destination) => destination;
+            public bool IsBypassed(Uri host) => true;
+        }
     }
 
     public sealed class SocketsHttpHandler_PublicAPIBehavior_Test

StandardSocketsHttpHandler/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

@@ -89,7 +89,17 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings)
                         restoreFlow = true;
                     }
 
-                    _cleaningTimer = new Timer(s => ((HttpConnectionPoolManager)s).RemoveStalePools(), this, Timeout.Infinite, Timeout.Infinite);
+                    // Create the timer.  Ensure the Timer has a weak reference to this manager; otherwise, it
+                    // can introduce a cycle that keeps the HttpConnectionPoolManager rooted by the Timer
+                    // implementation until the handler is Disposed (or indefinitely if it's not).
+                    _cleaningTimer = new Timer(s =>
+                    {
+                        var wr = (WeakReference<HttpConnectionPoolManager>)s;
+                        if (wr.TryGetTarget(out HttpConnectionPoolManager thisRef))
+                        {
+                            thisRef.RemoveStalePools();
+                        }
+                    }, new WeakReference<HttpConnectionPoolManager>(this), Timeout.Infinite, Timeout.Infinite);
                 }
                 finally
                 {