From 434fe7508dafd9e4cc865fa18b74dd1c6888552a Mon Sep 17 00:00:00 2001 From: Corcodel Iulia <31440913+iuliaco@users.noreply.github.com> Date: Fri, 22 Jul 2022 18:31:22 +0300 Subject: [PATCH] HTTP/3 Improved message for failing QUIC connection (#72374) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [#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 * Solving the rest of the coding review * Update src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs Co-authored-by: Natalia Kondratyeva Co-authored-by: iuliaco Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> Co-authored-by: Natalia Kondratyeva --- .../SocketsHttpHandler/HttpConnectionPool.cs | 31 ++++++++++--------- .../HttpClientHandlerTest.Http3.cs | 31 +++++++++++++++++++ 2 files changed, 48 insertions(+), 14 deletions(-) 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 41f3a2bc3a149..02baec7b17872 100644 --- 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 @@ internal sealed class HttpConnectionPool : IDisposable /// When an Alt-Svc authority fails due to 421 Misdirected Request, it is placed in the blocklist to be ignored /// for milliseconds. Initialized on first use. /// - private volatile HashSet? _altSvcBlocklist; + private volatile Dictionary? _altSvcBlocklist; private CancellationTokenSource? _altSvcBlocklistTimerCancellation; private volatile bool _altSvcEnabled = true; @@ -419,11 +419,11 @@ private object SyncObj // 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 @@ private async ValueTask GetHttp3ConnectionAsync(HttpRequestMess 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 @@ private async ValueTask GetHttp3ConnectionAsync(HttpRequestMess 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 @@ internal void HandleAltSvc(IEnumerable altSvcHeaderValues, TimeSpan? res 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 @@ private void ExpireAltSvcAuthority() /// /// Checks whether the given is on the currext Alt-Svc blocklist. + /// If it is, then it places the cause in the /// /// - 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; } + /// /// Blocklists an authority and resets the current authority back to origin. /// If the number of blocklisted authorities exceeds , @@ -1273,11 +1276,11 @@ private bool IsAltSvcBlocked(HttpAuthority authority) /// 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. /// - internal void BlocklistAuthority(HttpAuthority badAuthority) + internal void BlocklistAuthority(HttpAuthority badAuthority, Exception? exception = null) { Debug.Assert(badAuthority != null); - HashSet? altSvcBlocklist = _altSvcBlocklist; + Dictionary? altSvcBlocklist = _altSvcBlocklist; if (altSvcBlocklist == null) { @@ -1292,7 +1295,7 @@ internal void BlocklistAuthority(HttpAuthority badAuthority) altSvcBlocklist = _altSvcBlocklist; if (altSvcBlocklist == null) { - altSvcBlocklist = new HashSet(); + altSvcBlocklist = new Dictionary(); _altSvcBlocklistTimerCancellation = new CancellationTokenSource(); _altSvcBlocklist = altSvcBlocklist; } @@ -1303,7 +1306,7 @@ internal void BlocklistAuthority(HttpAuthority badAuthority) 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 61414a2e7af1f..af609cf8ec675 100644 --- 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 @@ public async Task Alpn_NonH3_NegotiationFailure() }; HttpRequestException ex = await Assert.ThrowsAsync(() => client.SendAsync(request).WaitAsync(TimeSpan.FromSeconds(10))); + Assert.IsType(ex.InnerException); clientDone.Release(); @@ -1062,6 +1063,36 @@ public async Task Alpn_NonH3_NegotiationFailure() 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(() => 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(() => 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);