Don't call into unsupported APIs on build targets which don't support quic #49261
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| using System.Net.Sockets; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.InteropServices; | ||
| using System.Runtime.Versioning; | ||
| using System.Security.Authentication; | ||
| using System.Text; | ||
| using System.Threading; | ||
|
|
@@ -119,7 +120,11 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK | |
| } | ||
|
|
||
| _http2Enabled = _poolManager.Settings._maxHttpVersion >= HttpVersion.Version20; | ||
| // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished | ||
| if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) | ||
| { | ||
| _http3Enabled = _poolManager.Settings._maxHttpVersion >= HttpVersion.Version30 && (_poolManager.Settings._quicImplementationProvider ?? QuicImplementationProviders.Default).IsSupported; | ||
| } | ||
|
|
||
| switch (kind) | ||
| { | ||
|
|
@@ -240,10 +245,14 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK | |
| _http3EncodedAuthorityHostHeader = QPackEncoder.EncodeLiteralHeaderFieldWithStaticNameReferenceToArray(H3StaticTable.Authority, hostHeader); | ||
| } | ||
|
|
||
| // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished | ||
| if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) | ||
| { | ||
| if (_http3Enabled) | ||
| { | ||
| _sslOptionsHttp3 = ConstructSslOptions(poolManager, sslHostName); | ||
| _sslOptionsHttp3.ApplicationProtocols = s_http3ApplicationProtocols; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -256,10 +265,22 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK | |
| if (NetEventSource.Log.IsEnabled()) Trace($"{this}"); | ||
| } | ||
|
|
||
| private static readonly List<SslApplicationProtocol> s_http3ApplicationProtocols = CreateHttp3ApplicationProtocols(); | ||
| private static readonly List<SslApplicationProtocol> s_http2ApplicationProtocols = new List<SslApplicationProtocol>() { SslApplicationProtocol.Http2, SslApplicationProtocol.Http11 }; | ||
| private static readonly List<SslApplicationProtocol> s_http2OnlyApplicationProtocols = new List<SslApplicationProtocol>() { SslApplicationProtocol.Http2 }; | ||
|
|
||
| private static List<SslApplicationProtocol> CreateHttp3ApplicationProtocols() | ||
| { | ||
| // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished | ||
| if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) | ||
| { | ||
| // TODO: Once the HTTP/3 versions are part of SslApplicationProtocol, see https://github.com/dotnet/runtime/issues/1293, move this back to field initialization. | ||
| return new List<SslApplicationProtocol>() { Http3Connection.Http3ApplicationProtocol31, Http3Connection.Http3ApplicationProtocol30, Http3Connection.Http3ApplicationProtocol29 }; | ||
| } | ||
|
|
||
| return null!; | ||
|
There was a problem hiding this comment. Isn't this lying to the compiler? If the field can be There was a problem hiding this comment. Once the H/3 ALPN versions are part of |
||
| } | ||
|
|
||
| private static SslClientAuthenticationOptions ConstructSslOptions(HttpConnectionPoolManager poolManager, string sslHostName) | ||
| { | ||
| Debug.Assert(sslHostName != null); | ||
|
|
@@ -352,24 +373,28 @@ public byte[] Http2AltSvcOriginUri | |
| } | ||
| } | ||
|
|
||
| // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished | ||
| if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) | ||
| { | ||
| // Either H3 explicitly requested or secured upgraded allowed. | ||
| if (_http3Enabled && (request.Version.Major >= 3 || (request.VersionPolicy == HttpVersionPolicy.RequestVersionOrHigher && IsSecure))) | ||
| { | ||
| HttpAuthority? authority = _http3Authority; | ||
| // H3 is explicitly requested, assume prenegotiated H3. | ||
| if (request.Version.Major >= 3 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower) | ||
| { | ||
| authority = authority ?? _originAuthority; | ||
| } | ||
| if (authority != null) | ||
| { | ||
| if (IsAltSvcBlocked(authority)) | ||
| { | ||
| return ValueTask.FromException<(HttpConnectionBase connection, bool isNewConnection)>( | ||
| new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 3))); | ||
| } | ||
|
|
||
| return GetHttp3ConnectionAsync(request, authority, cancellationToken); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -719,6 +744,10 @@ private void AddHttp2Connection(Http2Connection newConnection) | |
| } | ||
| } | ||
|
|
||
| // TODO: SupportedOSPlatform doesn't work for internal APIs https://github.com/dotnet/runtime/issues/51305 | ||
| [SupportedOSPlatform("windows")] | ||
| [SupportedOSPlatform("linux")] | ||
| [SupportedOSPlatform("macos")] | ||
| private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)> | ||
| GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) | ||
| { | ||
|
|
@@ -891,14 +920,18 @@ public async ValueTask<HttpResponseMessage> SendWithRetryAsync(HttpRequestMessag | |
| HandleAltSvc(altSvcHeaderValues, response.Headers.Age); | ||
| } | ||
|
|
||
| // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished | ||
| if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) | ||
| { | ||
| // If an Alt-Svc authority returns 421, it means it can't actually handle the request. | ||
| // An authority is supposed to be able to handle ALL requests to the origin, so this is a server bug. | ||
| // In this case, we blocklist the authority and retry the request at the origin. | ||
| if (response.StatusCode == HttpStatusCode.MisdirectedRequest && connection is Http3Connection h3Connection && h3Connection.Authority != _originAuthority) | ||
| { | ||
| response.Dispose(); | ||
| BlocklistAuthority(h3Connection.Authority); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| return response; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.IO; | ||
| using System.Net.Http; | ||
| using System.Threading.Tasks; | ||
|
|
||
| class Program | ||
| { | ||
| static async Task<int> Main(string[] args) | ||
|
There was a problem hiding this comment. I don't understand the intention of this test. I also don't understand why a trimming test is being included in a change concerning the Platform Compatibility Analyzer. There was a problem hiding this comment. We want to trim System.Net.Quic.dll as well. |
||
| { | ||
| using var client = new HttpClient(); | ||
| using var response = await client.GetAsync("https://www.microsoft.com"); | ||
| var result = await response.Content.ReadAsStringAsync(); | ||
| Console.WriteLine(result); | ||
|
|
||
| const string quicDll = "System.Net.Quic.dll"; | ||
| var quicDllExists = File.Exists(Path.Combine(AppContext.BaseDirectory, quicDll)); | ||
|
|
||
| // TODO: Replace with Platform-Guard Assertion Annotations once https://github.com/dotnet/runtime/issues/44922 is finished | ||
| if ((OperatingSystem.IsLinux() && !OperatingSystem.IsAndroid()) || OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) | ||
| { | ||
| Console.WriteLine($"Expected {quicDll} is {(quicDllExists ? "present - OK" : "missing - BAD")}."); | ||
| return quicDllExists ? 100 : -1; | ||
| } | ||
| else | ||
| { | ||
| Console.WriteLine($"Unexpected {quicDll} is {(quicDllExists ? "present - BAD" : "missing - OK")}."); | ||
| return quicDllExists ? -1 : 100; | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <Project DefaultTargets="Build"> | ||
| <Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props))" /> | ||
|
|
||
| <ItemGroup> | ||
| <TestConsoleAppSourceFiles Include="HttpClientTest.cs"> | ||
| <SkipOnTestRuntimes>browser-wasm</SkipOnTestRuntimes> | ||
|
There was a problem hiding this comment. Doesn't disabling this test on wasm basically invalidate it? This test was trying to ensure Quic gets trimmed on non-supported platforms. The only non-supported platform this test runs on is wasm. So I don't see the point of these tests. There was a problem hiding this comment. It crashes on wasm, I don't know how to fix that. But I'd love to still have this test if possible. There was a problem hiding this comment. The "headers" failure indicated that http-related JS APIs were being accessed inside of v8. Whether or not that indicates the trimming was successful is hard to say, but i would confidently assert that no code relying on the ability to issue http requests is going to pass inside the v8 shell, so for that test to pass it needs to have the HTTP bits entirely mocked or shimmed out There was a problem hiding this comment.
That's the eventual plan, yes. It currently isn't scheduled though. There was a problem hiding this comment. So should I remove it for now? If so, I'll do it in a follow up PR, no problem, just say so. There was a problem hiding this comment. It's fine to leave it. Once the other platforms come on line this will add value. |
||
| </TestConsoleAppSourceFiles> | ||
| </ItemGroup> | ||
|
|
||
| <Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.targets))" /> | ||
| </Project> | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Once this PR goes through, I plan to send a follow-up PR that utilizes the
<SupportedOSPlatforms>property instead of<IsWindowsSpecific>. jeffhandley@0f530be