Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid multiple exceptions at startup from MsQuic support tests #49973

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

stephentoub
Copy link
Member

Fixes #49918
cc: @brianrob, @scalablecory, @geoffkizer

Cory, Geoff, I'm not sure how to install msquic to test this. Could you either a) confirm CI will appropriately validate it, b) validate it, or c) tell me how to get/install the relevant dll I'd need to validate it?

@ghost
Copy link

ghost commented Mar 22, 2021

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

Issue Details

Fixes #49918
cc: @brianrob, @scalablecory, @geoffkizer

Cory, Geoff, I'm not sure how to install msquic to test this. Could you either a) confirm CI will appropriately validate it, b) validate it, or c) tell me how to get/install the relevant dll I'd need to validate it?

Author: stephentoub
Assignees: -
Labels:

area-System.Net

Milestone: -

@karelz
Copy link
Member

karelz commented Mar 22, 2021

Thanks @stephentoub!
Also adding @ManickaP @CarnaViire who are now actively working on msquic.
Won't it collide with @ManickaP's PR?

@ManickaP
Copy link
Member

I'd love to understand the problem before we dive into fixing it so that we can avoid making the same mistake in the future. I've posted some question in the original issue about the exception and its circumstances. I also have some questions about this PR:

Otherwise, this will clash with #49823. Depending on the priority of the original issue:

  • If not urgent, I'd prefer to get in my PR sooner, since this is smaller and thus easier to solve the conflicts.
  • If this needs to get in ASAP, then I'll do a review to minimize impact on [QUIC] API Update #49823.

Finally, to answer your question @stephentoub:

Could you either a) confirm CI will appropriately validate it, b) validate it, or c) tell me how to get/install the relevant dll I'd need to validate it?

a) Nope, CI won't validate: #49138
b) Me or @CarnaViire can do it
c) If you want to do it yourself, instructions are here: https://github.com/dotnet/runtimelab/tree/feature/System.Net.Experimental.MsQuic

@stephentoub
Copy link
Member Author

stephentoub commented Mar 22, 2021

Does this somehow relates to this PR: #49261?

Only tangentially. That PR is about changing System.Net.Http to avoid accessing APIs from System.Net.Quic on platforms where Quic is definitely not supported. This issue is about the impact of accessing those APIs (and in particular IsSupported) on platforms where it might be supported.

If not urgent, I'd prefer to get in my PR sooner, since this is smaller and thus easier to solve the conflicts.

It's not urgent. Feel free to cherry-pick this into your other PR if that makes things easier for you.

I'd love to understand the problem before we dive into fixing it so that we can avoid making the same mistake in the future.

Exceptions are expensive, in particular throwing and catching them, and way more so if a debugger is attached. We want exceptions to be "exceptional", not the common case.

Here, we're P/Invoking into MsQuicOpen from the msquic.dll library. If that library doesn't exist, that P/Invoke will generate a DllNotFoundException. The code is then catching that exception, and throwing a new NotSupportedException, which it then also catches and turns into setting IsSupported to false.

Since for the foreseeable future it's very likely that the common case on many systems will be that they don't have msquic.dll, and we're throwing two exceptions as part of computing IsSupported when it's not supported, we're putting multiple exceptions onto the common path, and a path that's often used as part of an app starting up (since it's used from HttpClient as part of determining whether HTTP/3 is available in SocketsHttpHandler's connection pool).

This PR avoids making such a P/Invoke by instead using NativeLibrary.TryLoad/TryGetExport to get MsQuicOpen, changing the code to not use exceptions for the control flow around determining whether IsSupported should be set to true.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this, I'll deal with the rebase. The change makes sense. I was not aware that we had enabled H3 by default.
Thanks.

@stephentoub
Copy link
Member Author

Ok. Thanks.

@stephentoub stephentoub merged commit 18ddd57 into dotnet:main Mar 24, 2021
@stephentoub stephentoub deleted the msquicopen branch March 24, 2021 21:24
@ghost ghost locked as resolved and limited conversation to collaborators Apr 23, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient.SendAsync Throws Two Exceptions When QUIC Is Not Supported
4 participants