Don't call into unsupported APIs on build targets which don't support quic #49261

marek-safar · 2021-03-06T11:55:34Z

ghost · 2021-03-06T11:55:41Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

The code could be extracted to HttpConnectionPool.quick.cs but not sure
it's worth the split as that would move the majority of the code.

Fixes #49201
Fixes #49187

Author:	marek-safar
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

stephentoub · 2021-03-06T12:19:35Z

Can we instead do this with a few strategically placed Http3.IsSupported calls, skipping the affected code paths and also enabling the linker to remove most of the relevant code on platforms where it's false? I'd like to avoid littering the code base with these ifdefs.

marek-safar · 2021-03-06T13:01:43Z

Can we instead do this with a few strategically placed Http3.IsSupported calls

That will not work with the unsupported analyzer warning but if we are fine to pragma that then it could work but won't be as effective size-wise.

stephentoub · 2021-03-06T13:17:06Z

if we are fine to pragma that then it could work

I'd be fine suppressing the analyzer for the whole assembly for the problematic build flavor until the analyzer can be taught to understand such guards. I thought there was an issue about this but I can't find it now. We're blessing this IsSupported pattern as a recommended way to enable portions of code to be enabled/disabled from project files and run-time switches, and for the linker to recognize... we should find a way for that to be factored into the platform analyzer as well, e.g. having the analyzer see that it's hard-coded to return a particular value, reading the project file to see if there's a corresponding switch, having some way to express to the analyzer a mapping between IsSupported and various platform values, etc.
cc: @jeffhandley, @buyaa-n, @eerhardt

won't be as effective size-wise

I would hope it's very close. Otherwise, seems we have larger issues to address.

… quic The code could be extracted to HttpConnectionPool.quick.cs but not sure it's worth the split as that would move the majority of the code. Fixes dotnet#49201 Fixes dotnet#49187

jeffhandley · 2021-03-06T18:33:40Z

The issue you're referring to @stephentoub is #44922 (comment). @buyaa-n had some preliminary designs for that, but it still needs more design effort. This can be tracked as another use case for platform guard assertions.

marek-safar · 2021-03-09T10:55:59Z

We're blessing this IsSupported pattern as a recommended way to enable portions of code to be enabled/disabled from project files

Updated the PR.

karelz · 2021-03-09T11:01:25Z

This is blocking @marek-safar's team -- it broke SocketsHttpHandler in Preview 2 on mobile platforms. They would like to fix it in Preview 3 - ideally by 3/19.
@stephentoub @scalablecory can you please help with the review to hit the date? Thanks!
cc @ManickaP @CarnaViire @geoffkizer

CarnaViire

LGTM, but please also wait for @scalablecory or @ManickaP reviews

ManickaP · 2021-03-09T14:54:47Z

How is splitting the code into partial classes and conditionally compile them different from the ifdefs? I thought that:

Can we instead do this with a few strategically placed Http3.IsSupported calls.

means that we can guard the HTTP/3 stuff in place without splitting the code. Or am I wrong?

I'd really like to understand this change and what the general consensus here is, so please bear with me.

stephentoub · 2021-03-09T14:58:01Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

-
-                    return GetHttp3ConnectionAsync(request, authority, cancellationToken);
-                }
+                var result = GetHttp3ConnectionAsync(request, cancellationToken);


Can you help me understand why all of these changes are needed?

My suggestion was to have just an IsHttp3Enabled or Http3.IsSupported property (the latter of which could be configured via the same mechanisms we use for other IsSupported properties) that was special-cased by platform. If the linker could see it was false, for example, then here GetHttp3ConnectionAsync shouldn't need to have been moved and stubbed out; it would never be invoked on platforms where it wasn't supported, and the linker would be able to remove it entirely, since all calls to it are guarded by IsHttp3Enabled/Http3.IsSupported.

The quic apis were called from more places than expected and the simplest way to automatically check that no PNSE APIs are never called is not to compile them (the analyzer does not work here). If it was only a few methods it'd be fine to put IsHttp3Enabled everywhere but how do you ensure the PNSE code is never called and no future PR break it?

how do you ensure the PNSE code is never called and no future PR break it?

Isn't that what the platform compatibility analyzer is for?

It does not work here see #49278 and its method body limited so that's a lot of additional annotations to bubble it up to right IsHttp3Enabled check.

ManickaP · 2021-03-15T12:20:55Z

I'm taking over of this PR. I'll try to make the change suggested by @stephentoub (i.e. with correctly placed if (IsSupported)...).

cc: @karelz @marek-safar

ManickaP

I moved the the problem one layer down to System.Net.Quic, which causes much less disturbance. The System.Net.Http stays as-is. Note that all the access to H3 code has already been guarded behind _http3Enabled, I just fixed that the initialization won't blow up even on Android etc.

src/libraries/System.Net.Http/tests/UnitTests/HttpClientTest.cs

ManickaP · 2021-03-24T21:02:06Z

src/libraries/System.Net.Quic/ref/System.Net.Quic.cs

@@ -6,121 +6,124 @@

 namespace System.Net.Quic
 {
-    public static class QuicImplementationProviders
+    public partial class QuicClientConnectionOptions


Regenerated ref sources. The => throw null; were causing the PNSE generator not to generate the stub.

marek-safar · 2021-03-24T21:44:02Z

...s/System.Net.Quic/src/System/Net/Quic/Implementations/NoQuic/NoQuicImplementationProvider.cs

+{
+    internal sealed class NoQuicImplementationProvider : QuicImplementationProvider
+    {
+        public override bool IsSupported => false;


The linker won't be able to see this logic so the change no longer fixes #46915 issue. It will also trigger a platform compatibility warning which is probably not desirable which cannot be fix without pragma.

The linker won't be able to see this logic so the change no longer fixes #46915 issue.

I'll try to fix that, but I need a way how to test it and confirm it work as it should. ATM, I don't even know how the desired result looks let alone be able to confirm it.

It will also trigger a platform compatibility warning

How can I test this?

ManickaP · 2021-04-13T19:33:14Z

It's not only about the attributes, they are made to be consistent with the code. See for instance this part of the commit:
c9904b6#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R124

buyaa-n · 2021-04-13T19:40:36Z

It's not only about the attributes, they are made to be consistent with the code. See for instance this part of the commit:
c9904b6#diff-bc2faad6c76ecb9b115e305763eb55ebfde74de1eee27ed549e5edaabb097a99R124

Right, that guard makes sense, i meant only the attribute would have no effect so better to be removed

jeffhandley · 2021-04-14T01:26:47Z

Right, that guard makes sense, i meant only the attribute would have no effect so better to be removed

I want to confirm we don't need to express that the API is supported on most flavors of Linux, but not on Android. Given this example, I interpret the intent to be:

This API is supported on Windows, macOS, and all flavors of Linux except Android
There could be API calls within this method that are not supported on Android, and that's OK

        [SupportedOSPlatform("windows")]
        [SupportedOSPlatform("linux")]
        [SupportedOSPlatform("macos")]
        [UnsupportedOSPlatform("android")]
        private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
            GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }

Since this is treated by the analyzer as an inconsistent list, I'm not sure how else we would capture that Android is an unsupported subset of Linux here. Did I understand the intent correctly, @ManickaP?

jeffhandley · 2021-04-14T01:59:56Z

eng/versioning.targets

+  <!-- Adds UnsupportedOSPlatform and SupportedOSPlatform attributes to the assembly when:
+      * At least one <UnsupportedOSPlatforms /> or <SupportedOSPlatforms /> has been specified


Note: Once this PR goes through, I plan to send a follow-up PR that utilizes the <SupportedOSPlatforms> property instead of <IsWindowsSpecific>. jeffhandley@0f530be

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs

ManickaP · 2021-04-14T09:50:05Z

@jeffhandley @buyaa-n
Are you saying that just this:

[SupportedOSPlatform("windows")]
[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }

means that it's supported on Linux but not supported on Android? Therefore, something like the following code should fail when build for Android:

if (OperatingSystem.IsAndroid())
{
    // Touching method that's supposed to be supported only on Linux, Windows, MacOS within an Android code.
    return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

If so, then it doesn't work. I just tried that and compilation finishes without an error. However, if I change the attributes to:

[SupportedOSPlatform("windows")]
[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
[UnsupportedOSPlatform("android")]
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }

I will get a compilation error on Android and only on Android for:

// Don't even need the if, the build will fail for Android and pass for anything else.
// if (OperatingSystem.IsAndroid())
{
    // Touching method that's supposed to be supported only on Linux, Windows, MacOS.
    return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

Which is exactly what I'm trying to achieve.

Edit: It doesn't work properly, this should fail for tvOS, iOS etc. as well, but doesn't. Seems like the analyzer doesn't do what I'd expect. SupportedOSPlatform attribute (even without the 'inconsistency') will not fail the build when the annotated class/method is called on some of the other OSes.

ManickaP · 2021-04-14T10:05:09Z

@jeffhandley

This API is supported on Windows, macOS, and all flavors of Linux except Android

Yes, only windows, osx and linux. It's not supported on mobile platforms therefore the !Android.

There could be API calls within this method that are not supported on Android, and that's OK

Sorry, I'm not following this question.

buyaa-n · 2021-04-14T17:06:11Z

Are you saying that just this:

[SupportedOSPlatform("windows")]
[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }

means that it's supported on Linux but not supported on Android?

Yes, [SupportedOSPlatform("windows"), SupportedOSPlatform("linux"), SupportedOSPlatform("macos")] means that it's supported on Linux but not supported on Android and it means GetHttp3ConnectionAsync(...) cannot be called/used from Android context/callsite, but guard methods are not used for evaluating call site, you could try IsAndroid() method from a call site that is reachable in Android the it should make sense

Therefore, something like the following code should fail when build for Android:

if (OperatingSystem.IsAndroid())
{
    // Touching method that's supposed to be supported only on Linux, Windows, MacOS within an Android code.
    return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

No, guard methods are not used for evaluating call site, call site supported by Android is when it has [SupportedOSPlatform("android")] attribute or has no any attributes (cross-platform) at any level (from API to assembly). For example:

[SupportedOSPlatform("android")]
public void GetConnection()
{
    // Here you should see the warning
    return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

buyaa-n · 2021-04-14T17:10:24Z

Edit: It doesn't work properly, this should fail for tvOS, iOS etc. as well, but doesn't. Seems like the analyzer doesn't do what I'd expect. SupportedOSPlatform attribute (even without the 'inconsistency') will not fail the build when the annotated class/method is called on some of the other OSes.

I am not sure how you are tested, if you just used a guard method for checking tvOS, iOS then it wouldn't work. Try them in cross-platform context at least, then using a guard method could make sense

ManickaP · 2021-04-14T17:31:35Z

Forget about the if (OperatingSystem.IsAndroid()), I was just trying to make the point. Imagine this scenario:

[SupportedOSPlatform("windows")]
[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }
    
public async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetConnection(...)
{
   ...
   // This line should cause a warning on Android/tvOS/iOS etc. but doesn't
   return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

Note that GetConnection is accessible everywhere, no attributes, no guards. This should cause the warning according to what you say, but doesn't. Also note that I'm cross-compiling for Android/tvOS/iOS etc. None of them raises any warnings.

buyaa-n · 2021-04-14T18:52:52Z

I see your point, that is actually an expected scenario, for example for $(NetCoreAppCurrent)-Android; targeted build MSBuild would add assembly-level [SupportedOSPlatform("Android")] attribute which makes/infers the assembly only reachable on Android. Now when the assembly is only reachable on Android adding [SupportedOSPlatform("windows"), SupportedOSPlatform("linux"), SupportedOSPlatform("macos")] attribute for an API inside that assembly has no effect (the child attributes are ignored by a child member annotation cannot widen the parent annotation rule), therefore called GetHttp3ConnectionAsync(...) will only have [SupportedOSPlatform("Android")] so there is no warning

[assembly:SupportedOSPlatform("Android")]
 ...
[SupportedOSPlatform("windows")]
[SupportedOSPlatform("linux")]
[SupportedOSPlatform("macos")]
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken) { ... }
    
public async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
    GetConnection(...)
{
   ...
   // there will be no warning as it all are supported on Android
   return GetHttp3ConnectionAsync(request, authority, cancellationToken);
}

It would only make sense when the API is referenced from outside

ManickaP · 2021-04-14T19:12:05Z

So it basically renders [SupportedOSPlatform(...)] unusable internally and I should have kept the Unsupported version Marek originally did.
And we shouldn't probably even add the support for it (https://github.com/dotnet/runtime/pull/49261/files#diff-3e71d76d1049d3878247187d660b7cab477a963020055effc4160ac4e85962cfR79-R83), because this is gonna cause many more headaches.

buyaa-n · 2021-04-14T19:29:43Z

So it basically renders [SupportedOSPlatform(...)] unusable internally and I should have kept the Unsupported version Marek originally did.
And we shouldn't probably even add the support for it (https://github.com/dotnet/runtime/pull/49261/files#diff-3e71d76d1049d3878247187d660b7cab477a963020055effc4160ac4e85962cfR79-R83), because this is gonna cause many more headaches.

I guess annotations are dedicated more for external users and the correct annotation for the longer run will be Supported, i think we should not annotate it just to see warnings internally, we might want to consider allowing child APIs to override assembly-level attribute CC @terrajobst

terrajobst · 2021-04-14T19:41:56Z

Maybe I'm missing something but why is there no warning? It seems the API that is being called is known to be OS specific and doesn't have support for Android. Or this is the case we explicitly made not-warn based on Marek's earlier feedback?

buyaa-n · 2021-04-14T21:01:38Z

Maybe I'm missing something but why is there no warning? It seems the API that is being called is known to be OS-specific and doesn't have support for Android.

@terrajobst because for Android targeted build MSBuild would add assembly level [SupportedOSPlatform("Android")] attribute which makes the assembly only reachable on Android. Now when the assembly is only reachable on Android adding [SupportedOSPlatform("windows"), SupportedOSPlatform("linux"), SupportedOSPlatform("macos")] attribute for an API inside that assembly has no effect (by the rule child member annotation cannot widen the parent annotation)), therefore calling GetHttp3ConnectionAsync(...) will not warn, adding [UnsupportedOSPlatform("Android")] to the API is working because it is not widening the parent support. The example is in my previous comment. I understand @ManickaP's intention for using the working attribute but the nature of the API is it is only supported on windows, linux and macos , so we should use the Supported only list, which would work just fine from outside of this project. It is not cool to not have warnings internally i agree, we might want special case to allow overriding this assembly-level attribute

Or this is the case we explicitly made not-warn based on Marek's earlier feedback?

It is not exactly the same issue but introduced with the same cause (SDK adding a supported attribute for the target platform)

marek-safar · 2021-04-15T12:05:03Z

LGTM

eerhardt · 2021-04-15T18:09:29Z

src/libraries/System.Net.Http/tests/TrimmingTests/System.Net.Http.TrimmingTests.proj

+
+  <ItemGroup>
+    <TestConsoleAppSourceFiles Include="HttpClientTest.cs">
+      <SkipOnTestRuntimes>browser-wasm</SkipOnTestRuntimes>


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.

It crashes on wasm, I don't know how to fix that. But I'd love to still have this test if possible.
I also thought we plan to widen the trimming test for platforms like Android, iOS, for which we definitely should have this test.

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

we plan to widen the trimming test for platforms like Android, iOS,

That's the eventual plan, yes. It currently isn't scheduled though.

So should I remove it for now? If so, I'll do it in a follow up PR, no problem, just say so.

It's fine to leave it. Once the other platforms come on line this will add value.

dotnet-issue-labeler bot added the area-System.Net.Http label Mar 6, 2021

marek-safar force-pushed the quick2 branch from 470cd9d to 0879429 Compare March 6, 2021 16:30

Don't call into unsupported APIs on build targets which don't support…

ba66ddb

… quic The code could be extracted to HttpConnectionPool.quick.cs but not sure it's worth the split as that would move the majority of the code. Fixes dotnet#49201 Fixes dotnet#49187

marek-safar force-pushed the quick2 branch from 0879429 to ba66ddb Compare March 6, 2021 16:47

marek-safar force-pushed the quick2 branch 2 times, most recently from 031a02a to 462934b Compare March 9, 2021 08:16

if-def less version

90b4e11

marek-safar force-pushed the quick2 branch from 462934b to 90b4e11 Compare March 9, 2021 09:36

karelz requested review from scalablecory, ManickaP and CarnaViire March 9, 2021 10:54

karelz added this to the 6.0.0 milestone Mar 9, 2021

CarnaViire approved these changes Mar 9, 2021

View reviewed changes

stephentoub reviewed Mar 9, 2021

View reviewed changes

ManickaP self-assigned this Mar 15, 2021

ManickaP mentioned this pull request Mar 22, 2021

Avoid multiple exceptions at startup from MsQuic support tests #49973

Merged

ManickaP added 2 commits March 24, 2021 21:31

Merge branch 'main' into quick2

308b2f5

Quic IsSupported working and not throwing for AnyOS.

de703d5

ManickaP reviewed Mar 24, 2021

View reviewed changes

marek-safar commented Mar 24, 2021

View reviewed changes

Fix for running the newly added UnitTest.

b50a11f

jeffhandley reviewed Apr 14, 2021

View reviewed changes

marek-safar commented Apr 14, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs Show resolved Hide resolved

Fix for Android, which returns true for IsLinux().

e969f29

ManickaP force-pushed the quick2 branch from c9904b6 to e969f29 Compare April 14, 2021 09:39

ManickaP added 2 commits April 15, 2021 13:20

Skip failing trimming test on wasm

f8d3506

Removed additional UnsupportedOS attributes, referenced issue.

b26e3df

This was referenced Apr 15, 2021

Annotate unsupported APIs in System.Net #50530

Closed

[QUIC] Improve MSQuic OS Version detection #32575

Closed

ManickaP merged commit d4818a3 into dotnet:main Apr 15, 2021

ManickaP mentioned this pull request Apr 15, 2021

[CA1416] Allow child APIs attribute override assembly attribute in targeted builds of cross-platform assemblies dotnet/roslyn-analyzers#5023

Closed

eerhardt reviewed Apr 15, 2021

View reviewed changes

karelz mentioned this pull request Apr 16, 2021

[main] Update dependencies from dotnet/efcore dotnet/runtime dotnet/emsdk dotnet/aspnetcore#31748

Merged

jeffhandley mentioned this pull request Apr 17, 2021

Refactor the platform support attribute generation in the build #51450

Merged

filipnavara mentioned this pull request May 4, 2021

System.Xml.RW.XmlReader.Tests fails on iOS/tvOS #51391

Closed

MaximLipnin mentioned this pull request May 12, 2021

Annotate APIs throwing PNSE on iOS/tvOS/MacCatalyst #47910

Closed

27 tasks

ghost locked as resolved and limited conversation to collaborators May 16, 2021

		<!-- Adds UnsupportedOSPlatform and SupportedOSPlatform attributes to the assembly when:
		* At least one <UnsupportedOSPlatforms /> or <SupportedOSPlatforms /> has been specified

Don't call into unsupported APIs on build targets which don't support quic #49261

Don't call into unsupported APIs on build targets which don't support quic #49261

Conversation

marek-safar commented Mar 6, 2021 • edited Loading

ghost commented Mar 6, 2021

stephentoub commented Mar 6, 2021 • edited Loading

marek-safar commented Mar 6, 2021

stephentoub commented Mar 6, 2021

jeffhandley commented Mar 6, 2021 • edited Loading

marek-safar commented Mar 9, 2021

karelz commented Mar 9, 2021

CarnaViire left a comment

Choose a reason for hiding this comment

ManickaP commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP commented Mar 15, 2021

ManickaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marek-safar Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP commented Apr 13, 2021

buyaa-n commented Apr 13, 2021 • edited Loading

jeffhandley commented Apr 14, 2021

Choose a reason for hiding this comment

ManickaP commented Apr 14, 2021 • edited Loading

ManickaP commented Apr 14, 2021

buyaa-n commented Apr 14, 2021 • edited Loading

buyaa-n commented Apr 14, 2021

ManickaP commented Apr 14, 2021 • edited Loading

buyaa-n commented Apr 14, 2021 • edited Loading

ManickaP commented Apr 14, 2021 • edited Loading

buyaa-n commented Apr 14, 2021 • edited Loading

terrajobst commented Apr 14, 2021

buyaa-n commented Apr 14, 2021 • edited Loading

marek-safar commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marek-safar commented Mar 6, 2021 •

edited

Loading

stephentoub commented Mar 6, 2021 •

edited

Loading

jeffhandley commented Mar 6, 2021 •

edited

Loading

marek-safar Mar 24, 2021 •

edited

Loading

buyaa-n commented Apr 13, 2021 •

edited

Loading

ManickaP commented Apr 14, 2021 •

edited

Loading

buyaa-n commented Apr 14, 2021 •

edited

Loading

ManickaP commented Apr 14, 2021 •

edited

Loading

buyaa-n commented Apr 14, 2021 •

edited

Loading

ManickaP commented Apr 14, 2021 •

edited

Loading

buyaa-n commented Apr 14, 2021 •

edited

Loading

buyaa-n commented Apr 14, 2021 •

edited

Loading