Fix NetworkInformation / execution context usage for HTTP/3 and Alt-Svc (#32621)

Fix: leaking NetworkAddressChangedEventHandler and related WeakReference if user does not Dispose HttpClient. Resolves #32526.
Fix: using HttpClient from within a suppressed execution context. Add test for this. Resolves #32524.
Fix: capturing excessive execution context in Alt-Svc expiration timer.
Optimization: do not monitor for network changes until a non-persistent Alt-Svc is used.

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
index 0a65a9a..d8f25b7 100644 (file)
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
@@ -857,14 +857,28 @@ namespace System.Net.Http
                     {
                         var thisRef = new WeakReference<HttpConnectionPool>(this);
 
-                        _authorityExpireTimer = new Timer(o =>
+                        bool restoreFlow = false;
+                        try
                         {
-                            var wr = (WeakReference<HttpConnectionPool>)o;
-                            if (wr.TryGetTarget(out HttpConnectionPool @this))
+                            if (!ExecutionContext.IsFlowSuppressed())
                             {
-                                @this.ExpireAltSvcAuthority();
+                                ExecutionContext.SuppressFlow();
+                                restoreFlow = true;
                             }
-                        }, thisRef, nextAuthorityMaxAge, Timeout.InfiniteTimeSpan);
+
+                            _authorityExpireTimer = new Timer(o =>
+                            {
+                                var wr = (WeakReference<HttpConnectionPool>)o;
+                                if (wr.TryGetTarget(out HttpConnectionPool @this))
+                                {
+                                    @this.ExpireAltSvcAuthority();
+                                }
+                            }, thisRef, nextAuthorityMaxAge, Timeout.InfiniteTimeSpan);
+                        }
+                        finally
+                        {
+                            if (restoreFlow) ExecutionContext.RestoreFlow();
+                        }
                     }
                     else
                     {
@@ -874,6 +888,11 @@ namespace System.Net.Http
                     _http3Authority = nextAuthority;
                     _persistAuthority = nextAuthorityPersist;
                 }
+
+                if (!nextAuthorityPersist)
+                {
+                    _poolManager.StartMonitoringNetworkChanges();
+                }
             }
         }
 

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs
index 04a0cf8..d7cf040 100644 (file)
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs
@@ -44,7 +44,7 @@ namespace System.Net.Http
         private readonly IWebProxy _proxy;
         private readonly ICredentials _proxyCredentials;
 
-        private NetworkAddressChangedEventHandler _networkChangedDelegate;
+        private NetworkChangeCleanup _networkChangeCleanup;
 
         /// <summary>
         /// Keeps track of whether or not the cleanup timer is running. It helps us avoid the expensive
@@ -131,13 +131,23 @@ namespace System.Net.Http
                     _proxyCredentials = _proxy.Credentials ?? settings._defaultProxyCredentials;
                 }
             }
+        }
+
+        /// <summary>
+        /// Starts monitoring for network changes. Upon a change, <see cref="HttpConnectionPool.OnNetworkChanged"/> will be
+        /// called for every <see cref="HttpConnectionPool"/> in the <see cref="HttpConnectionPoolManager"/>.
+        /// </summary>
+        public void StartMonitoringNetworkChanges()
+        {
+            if (_networkChangeCleanup != null)
+            {
+                return;
+            }
 
             // Monitor network changes to invalidate Alt-Svc headers.
             // A weak reference is used to avoid NetworkChange.NetworkAddressChanged keeping a non-disposed connection pool alive.
             var poolsRef = new WeakReference<ConcurrentDictionary<HttpConnectionKey, HttpConnectionPool>>(_pools);
-            NetworkAddressChangedEventHandler networkChangedDelegate = null;
-
-            networkChangedDelegate = delegate
+            NetworkAddressChangedEventHandler networkChangedDelegate = delegate
             {
                 if (poolsRef.TryGetTarget(out ConcurrentDictionary<HttpConnectionKey, HttpConnectionPool> pools))
                 {
@@ -146,21 +156,50 @@ namespace System.Net.Http
                         pool.OnNetworkChanged();
                     }
                 }
-                else
-                {
-                    // Our pools were GCed; user did not dispose the handler.
-                    NetworkChange.NetworkAddressChanged -= networkChangedDelegate;
-                }
             };
 
-            _networkChangedDelegate = networkChangedDelegate;
+            var cleanup = new NetworkChangeCleanup(networkChangedDelegate);
+
+            if (Interlocked.CompareExchange(ref _networkChangeCleanup, cleanup, null) != null)
+            {
+                // We lost a race, another thread already started monitoring.
+                GC.SuppressFinalize(cleanup);
+                return;
+            }
 
-            using (ExecutionContext.SuppressFlow())
+            if (!ExecutionContext.IsFlowSuppressed())
+            {
+                using (ExecutionContext.SuppressFlow())
+                {
+                    NetworkChange.NetworkAddressChanged += networkChangedDelegate;
+                }
+            }
+            else
             {
                 NetworkChange.NetworkAddressChanged += networkChangedDelegate;
             }
         }
 
+        private sealed class NetworkChangeCleanup : IDisposable
+        {
+            private readonly NetworkAddressChangedEventHandler _handler;
+
+            public NetworkChangeCleanup(NetworkAddressChangedEventHandler handler)
+            {
+                _handler = handler;
+            }
+
+            // If user never disposes the HttpClient, use finalizer to remove from NetworkChange.NetworkAddressChanged.
+            // _handler will be rooted in NetworkChange, so should be safe to use here.
+            ~NetworkChangeCleanup() => NetworkChange.NetworkAddressChanged -= _handler;
+
+            public void Dispose()
+            {
+                NetworkChange.NetworkAddressChanged -= _handler;
+                GC.SuppressFinalize(this);
+            }
+        }
+
         public HttpConnectionSettings Settings => _settings;
         public ICredentials ProxyCredentials => _proxyCredentials;
 
@@ -377,11 +416,7 @@ namespace System.Net.Http
                 pool.Value.Dispose();
             }
 
-            if (_networkChangedDelegate != null)
-            {
-                NetworkChange.NetworkAddressChanged -= _networkChangedDelegate;
-                _networkChangedDelegate = null;
-            }
+            _networkChangeCleanup?.Dispose();
         }
 
         /// <summary>Sets <see cref="_cleaningTimer"/> and <see cref="_timerIsRunning"/> based on the specified timeout.</summary>

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
index c0c99aa..fe7560d 100644 (file)
--- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
+++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
@@ -26,6 +26,24 @@ namespace System.Net.Http.Functional.Tests
     {
         public SocketsHttpHandler_HttpClientHandler_Asynchrony_Test(ITestOutputHelper output) : base(output) { }
 
+        [Fact]
+        public async Task ExecutionContext_Suppressed_Success()
+        {
+            await LoopbackServerFactory.CreateClientAndServerAsync(
+                uri => Task.Run(() =>
+                {
+                    using (ExecutionContext.SuppressFlow())
+                    using (HttpClient client = CreateHttpClient())
+                    {
+                        client.GetStringAsync(uri).GetAwaiter().GetResult();
+                    }
+                }),
+                async server =>
+                {
+                    await server.AcceptConnectionSendResponseAndCloseAsync();
+                });
+        }
+
         [OuterLoop("Relies on finalization")]
         [Fact]
         public async Task ExecutionContext_HttpConnectionLifetimeDoesntKeepContextAlive()