Remove premature QuicListener.IsSupported check #44755
Conversation
- There's no reason to pay the cost of this check when HTTP/3 is not enabled
halter73
requested review from
Tratcher,
BrennanConroy and
JamesNK
as code owners
| @@ -36,7 +36,7 @@ internal sealed class QuicConnectionListener : IMultiplexedConnectionListener, I | |||
| { | |||
| if (!QuicListener.IsSupported) | |||
| { | |||
| throw new NotSupportedException("QUIC is not supported or enabled on this platform. See https://aka.ms/aspnet/kestrel/http3reqs for details."); | |||
| throw new NotSupportedException("QUIC and HTTP/3 are not supported or enabled on this platform. See https://aka.ms/aspnet/kestrel/http3reqs for details."); | |||
There was a problem hiding this comment.
Is there a unit test for this error? We could have a test that always runs that checks for QuicListener.IsSupported and expects an error if it is false.
| @@ -20,15 +19,10 @@ public static class WebHostBuilderQuicExtensions | |||
| /// <returns>The <see cref="IWebHostBuilder"/>.</returns> | |||
| public static IWebHostBuilder UseQuic(this IWebHostBuilder hostBuilder) | |||
| { | |||
| if (QuicListener.IsSupported) | |||
| return hostBuilder.ConfigureServices(services => | |||
There was a problem hiding this comment.
The presence of a registered multiplexed transport factory is tested when figuring out if an endpoint supports HTTP/3. For example:
There was a problem hiding this comment.
Good point. This isn't relevant if you have HTTP/3 disabled which is the .NET 7 default. Or if you have only HTTP/3 enabled which would throw either way.
But if you have HttpProtocols.Http1AndHttp2AndHttp3 configured which is the default in main, we have to pay the cost of the check regardless 😢. Do you think we could leverage your new CanBind API (#44537) for checking this in .NET 8?
If we backport this to 7 (which I still think we should do), we might just have to swallow the NotSupportedException thrown from BindAsync in the Http1AndHttp2AndHttp3 case. Fortunately, that's not the default in 7, and I don't see many people manually setting that unless they are running on a platform that supports QUIC.
There was a problem hiding this comment.
I think we should ask the S.N.Q team to look at improving QuicListener.IsSupported performance. See if it can be changed to avoid loading msquic.
There was a problem hiding this comment.
This isn't relevant if you have HTTP/3 disabled which is the .NET 7 default
dotnet/runtime#73153 enabled HTTP/3 for HttpClient by default for 7.0 rc1.
There was a problem hiding this comment.
dotnet/runtime#73153 enabled HTTP/3 for HttpClient by default for 7.0 rc1.
The developer still has to opt-in by changing the request version or policy.
…sions.cs Co-authored-by: James Newton-King <james@newtonking.com>
FYI - Loading msquic was showing up in a profile when I was measuring the startup of a NativeAOT'd ASP.NET app. A NativeAOT'd app is already roughly 5-10x faster than a "regular" ASP.NET app's startup. I wouldn't use my scenario or measurements alone as justification for changes here (especially if we are considering backporting the change). It would be good to verify how impactful this is for fully supported configurations. |
|
Not quite what we hoped for, but better than nothing. |
|
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
ghost
added
the
pending-ci-rerun
amcasey
added
area-networking
There's no reason to pay the cost of this check when HTTP/3 is not enabled. I was going to file an issue, but the fix was so simple, I just went ahead and made this PR. We can still discuss the merits here.
This means that a
IMultiplexedConnectionListenerFactoryalways gets added to the service collection, but the upside is that we only checkQuicListener.IsSupportedwhen Kestrel actually tries to bind with QUIC which means that HTTP/3 has been opted into. Today,QuicListener.IsSupportedis always checked and @eerhardt pointed out this hurts startup performance pretty significantly.I think we should consider backporting this to 7.0.1.
@JamesNK