don't load MsQuic unless needed by HttpClient #83494
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis change defers calling The check is done later if we have request eligible for H3 upgrade in For that reason, I verified that simple HttpClient app no longer calls
|
| if (QuicConnection.IsSupported) | ||
| { | ||
| if (_sslOptionsHttp3 == null) | ||
| { | ||
| // deferred creation | ||
| SslClientAuthenticationOptions sslOptionsHttp3 = ConstructSslOptions(_poolManager, _sslOptionsHttp11!.TargetHost!); | ||
| sslOptionsHttp3.ApplicationProtocols = s_http3ApplicationProtocols; | ||
| _sslOptionsHttp3 = sslOptionsHttp3; | ||
| } | ||
|
|
||
| response = await TrySendUsingHttp3Async(request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| _altSvcEnabled = false; | ||
| _http3Enabled = false; | ||
| } |
There was a problem hiding this comment.
This can race, are we sure that it doesn't cause any issues?
There was a problem hiding this comment.
I don't think it would but here is my thoughts. There are three properties that were originally set in constructor.
_http3Enabled could be false when QuicConnection.IsSupported is false. And that can flip _altSvcEnabled to false from default true and we would create _sslOptionsHttp3 if needed.
With the old code, _sslOptionsHttp3 would be allocated exactly once. Here there is chance that you more than one instance on race but it will converge once _sslOptionsHttp3 propagates to all threads. I feel that would be rare and still safe so I did not add any synchronization to make it cheap for the common case. I use local variable as the creation is not atomic but updating the reference should be.
_altSvcEnabled is true by default and as far as I understand it is really just optimization. So we may do little bit more work but it also seems like the alt headers are unlikely in traditional HTTP exchange.
Lastly, _http3Enabled is primarily used in the block above guarding invocation of TrySendUsingHttp3Async.
With that we would call QuicConnection.IsSupported more often instead of having local static variable. I was wondering fig I should do some Lazy variable but decided not as I feel it should be OK e.g. the call should be cheap once MsQuic is loaded. This is also where the crux of the change is e.g. avoiding calls to Quic until we have real candidate request.
It would be great to check my reasoning.
There was a problem hiding this comment.
The only problem I see from quick glance is possible double overwriting of _sslOptionsHttp3, (slower thread wins), but that does seem harmless.
@ManickaP What other race do you see there?
There was a problem hiding this comment.
If constructing the _sslOptionsHttp3 is cheap than we could keep it in the ctor and keep it readonly. That shouldn't have any effect on initializing S.N.Quic and MsQuic. Unless we really care about this being instantiated in vain per pool.
One thing I can think about is that the writes (_altSvcEnabled and _http3Enabled) can get reordered or that other threads might take a while to see the new values, as the writes are neither volatile nor in any construct forcing a memory barrier. I didn't investigate whether that can cause any issue, just pointing that out and making sure it was thought through.
There was a problem hiding this comment.
I'm not sure what readonly really gives us. Since H/3 is rare IMHO we would take unnecedsary memory and that seems like wrong tradeoff to me. (even if the allocation is somewhat cheap) Any thoughts on this @stephentoub ?
For the _http3Enabled other threads would call QuicConnection.IsSupported and may come to same conclusion if this comes back as false. That leaves _altSvcEnabled but that really seems like optimization to me. But I'm not 100% sure and I don't know how to test it.
There was a problem hiding this comment.
Sorry, I missed being tagged.
The lazy allocation here looks fine. This doesn't look technically necessary, but just to avoid any confusion, I'd suggest publishing a single instance only, changing:
_sslOptionsHttp3 = sslOptionsHttp3;to
Interlocked.CompareExchange(ref _sslOptionsHttp3, sslOptionsHttp3, null);so that even if there's a race every use sees the same published instance of the options object.
|
It seems like the I think the reorder would be ok with caveats mentioned above. I can watch it for while to make sure we do not see any bad outcome from it. |
ghost
locked as resolved and limited conversation to collaborators
5 participants
This change defers calling
QuicConnection.IsSupported(and loading MsQuic) fromHttpConnectionPollconstructor.It still does some selection and disables H3 when not applicable.
The check is done later if we have request eligible for H3 upgrade in
SendWithVersionDetectionAndRetryAsync.That is guarded by
IsHttp3Supportedfor trimming, by_http3Enabledwith existing checks and by deferredQuicConnection.IsSupportedfor actually platform MsQuic availability check.For that reason,
_http3Enabledand_sslOptionsHttp3are no longerreadonly.I verified that simple HttpClient app no longer calls
QuicConnection.IsSupportedso MsQuic is not loaded.fixes #76445