HTTP/3 Improved message for failing QUIC connection (#72374)

* [#70949 issue] Made test and added inner exception for uninformative
message

* Apply suggestions from code review

Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>
* Solving the rest of the coding review

* Update src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>
Co-authored-by: iuliaco <t-icorcodel@microsoft.com>
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>

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 41f3a2b..02baec7 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
@@ -47,7 +47,7 @@ namespace System.Net.Http
         /// When an Alt-Svc authority fails due to 421 Misdirected Request, it is placed in the blocklist to be ignored
         /// for <see cref="AltSvcBlocklistTimeoutInMilliseconds"/> milliseconds. Initialized on first use.
         /// </summary>
-        private volatile HashSet<HttpAuthority>? _altSvcBlocklist;
+        private volatile Dictionary<HttpAuthority, Exception?>? _altSvcBlocklist;
         private CancellationTokenSource? _altSvcBlocklistTimerCancellation;
         private volatile bool _altSvcEnabled = true;
 
@@ -419,11 +419,11 @@ namespace System.Net.Http
         // If not, then it must be unavailable at the moment; we will detect this and ensure it is not added back to the available pool.
 
         [DoesNotReturn]
-        private static void ThrowGetVersionException(HttpRequestMessage request, int desiredVersion)
+        private static void ThrowGetVersionException(HttpRequestMessage request, int desiredVersion, Exception? inner = null)
         {
             Debug.Assert(desiredVersion == 2 || desiredVersion == 3);
 
-            throw new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, desiredVersion));
+            throw new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, desiredVersion), inner);
         }
 
         private bool CheckExpirationOnGet(HttpConnectionBase connection)
@@ -889,7 +889,7 @@ namespace System.Net.Http
                     if (NetEventSource.Log.IsEnabled()) Trace($"QUIC connection failed: {e}");
 
                     // Disables HTTP/3 until server announces it can handle it via Alt-Svc.
-                    BlocklistAuthority(authority);
+                    BlocklistAuthority(authority, e);
                     throw;
                 }
 
@@ -940,9 +940,10 @@ namespace System.Net.Http
                     return null;
                 }
 
-                if (IsAltSvcBlocked(authority))
+                Exception? reasonException;
+                if (IsAltSvcBlocked(authority, out reasonException))
                 {
-                    ThrowGetVersionException(request, 3);
+                    ThrowGetVersionException(request, 3, reasonException);
                 }
 
                 long queueStartingTimestamp = HttpTelemetry.Log.IsEnabled() ? Stopwatch.GetTimestamp() : 0;
@@ -1148,8 +1149,7 @@ namespace System.Net.Http
                     if (nextAuthority == null && value != null && value.AlpnProtocolName == "h3")
                     {
                         var authority = new HttpAuthority(value.Host ?? _originAuthority!.IdnHost, value.Port);
-
-                        if (IsAltSvcBlocked(authority))
+                        if (IsAltSvcBlocked(authority, out _))
                         {
                             // Skip authorities in our blocklist.
                             continue;
@@ -1246,20 +1246,23 @@ namespace System.Net.Http
 
         /// <summary>
         /// Checks whether the given <paramref name="authority"/> is on the currext Alt-Svc blocklist.
+        /// If it is, then it places the cause in the <paramref name="reasonException"/>
         /// </summary>
         /// <seealso cref="BlocklistAuthority" />
-        private bool IsAltSvcBlocked(HttpAuthority authority)
+        private bool IsAltSvcBlocked(HttpAuthority authority, out Exception? reasonException)
         {
             if (_altSvcBlocklist != null)
             {
                 lock (_altSvcBlocklist)
                 {
-                    return _altSvcBlocklist.Contains(authority);
+                    return _altSvcBlocklist.TryGetValue(authority, out reasonException);
                 }
             }
+            reasonException = null;
             return false;
         }
 
+
         /// <summary>
         /// Blocklists an authority and resets the current authority back to origin.
         /// If the number of blocklisted authorities exceeds <see cref="MaxAltSvcIgnoreListSize"/>,
@@ -1273,11 +1276,11 @@ namespace System.Net.Http
         /// For now, the spec states alternate authorities should be able to handle ALL requests, so this
         /// is treated as an exceptional error by immediately blocklisting the authority.
         /// </remarks>
-        internal void BlocklistAuthority(HttpAuthority badAuthority)
+        internal void BlocklistAuthority(HttpAuthority badAuthority, Exception? exception = null)
         {
             Debug.Assert(badAuthority != null);
 
-            HashSet<HttpAuthority>? altSvcBlocklist = _altSvcBlocklist;
+            Dictionary<HttpAuthority, Exception?>? altSvcBlocklist = _altSvcBlocklist;
 
             if (altSvcBlocklist == null)
             {
@@ -1292,7 +1295,7 @@ namespace System.Net.Http
                     altSvcBlocklist = _altSvcBlocklist;
                     if (altSvcBlocklist == null)
                     {
-                        altSvcBlocklist = new HashSet<HttpAuthority>();
+                        altSvcBlocklist = new Dictionary<HttpAuthority, Exception?>();
                         _altSvcBlocklistTimerCancellation = new CancellationTokenSource();
                         _altSvcBlocklist = altSvcBlocklist;
                     }
@@ -1303,7 +1306,7 @@ namespace System.Net.Http
 
             lock (altSvcBlocklist)
             {
-                added = altSvcBlocklist.Add(badAuthority);
+                added = altSvcBlocklist.TryAdd(badAuthority, exception);
 
                 if (added && altSvcBlocklist.Count >= MaxAltSvcIgnoreListSize && _altSvcEnabled)
                 {

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
index 61414a2..af609cf 100644 (file)
--- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
+++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
@@ -1054,6 +1054,7 @@ namespace System.Net.Http.Functional.Tests
                 };
 
                 HttpRequestException ex = await Assert.ThrowsAsync<HttpRequestException>(() => client.SendAsync(request).WaitAsync(TimeSpan.FromSeconds(10)));
+
                 Assert.IsType<AuthenticationException>(ex.InnerException);
 
                 clientDone.Release();
@@ -1062,6 +1063,36 @@ namespace System.Net.Http.Functional.Tests
             await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(200_000);
         }
 
+        [Fact]
+        public async Task Alpn_NonH3_FailureEstablishConnection()
+        {
+            var options = new Http3Options() { Alpn = "h3-29" }; // anything other than "h3"
+            using Http3LoopbackServer server = CreateHttp3LoopbackServer(options);
+
+            using HttpClient client = CreateHttpClient();
+            using HttpRequestMessage request = new()
+            {
+                Method = HttpMethod.Get,
+                RequestUri = server.Address,
+                Version = HttpVersion30,
+                VersionPolicy = HttpVersionPolicy.RequestVersionExact
+            };
+            using HttpRequestMessage request2 = new()
+            {
+                Method = HttpMethod.Get,
+                RequestUri = server.Address,
+                Version = HttpVersion30,
+                VersionPolicy = HttpVersionPolicy.RequestVersionExact
+            };
+            HttpRequestException ex = await Assert.ThrowsAsync<HttpRequestException>(() => client.SendAsync(request).WaitAsync(TimeSpan.FromSeconds(10)));
+
+            // second request should throw the same exception as inner as the first one
+            HttpRequestException ex2 = await Assert.ThrowsAsync<HttpRequestException>(() => client.SendAsync(request2).WaitAsync(TimeSpan.FromSeconds(10)));
+
+            Assert.Equal(ex, ex2.InnerException);
+        }
+
+
         private SslApplicationProtocol ExtractMsQuicNegotiatedAlpn(Http3LoopbackConnection loopbackConnection)
         {
             FieldInfo quicConnectionField = loopbackConnection.GetType().GetField("_connection", BindingFlags.Instance | BindingFlags.NonPublic);