Skip to content

Commit

Permalink
HTTP/3 Improved message for failing QUIC connection (#72374)
Browse files Browse the repository at this point in the history 
* [#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>
  • Loading branch information
@iuliaco @ManickaP @CarnaViire
4 people authored Jul 22, 2022
1 parent 555a791 commit 434fe75
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 14 deletions.
31 changes: 17 additions & 14 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <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;

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -889,7 +889,7 @@ private async ValueTask<Http3Connection> 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;
}

Expand Down Expand Up @@ -940,9 +940,10 @@ private async ValueTask<Http3Connection> 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;
Expand Down Expand Up @@ -1148,8 +1149,7 @@ internal void HandleAltSvc(IEnumerable<string> 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;
Expand Down Expand Up @@ -1246,20 +1246,23 @@ private void ExpireAltSvcAuthority()

/// <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"/>,
Expand All @@ -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.
/// </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)
{
Expand All @@ -1292,7 +1295,7 @@ internal void BlocklistAuthority(HttpAuthority badAuthority)
altSvcBlocklist = _altSvcBlocklist;
if (altSvcBlocklist == null)
{
altSvcBlocklist = new HashSet<HttpAuthority>();
altSvcBlocklist = new Dictionary<HttpAuthority, Exception?>();
_altSvcBlocklistTimerCancellation = new CancellationTokenSource();
_altSvcBlocklist = altSvcBlocklist;
}
Expand All @@ -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)
{
Expand Down
31 changes: 31 additions & 0 deletions src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,7 @@ public async Task Alpn_NonH3_NegotiationFailure()
};
HttpRequestException ex = await Assert.ThrowsAsync<HttpRequestException>(() => client.SendAsync(request).WaitAsync(TimeSpan.FromSeconds(10)));
Assert.IsType<AuthenticationException>(ex.InnerException);
clientDone.Release();
Expand All @@ -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<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);
Expand Down

0 comments on commit 434fe75

Please sign in to comment.