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

[release/6.0] Close MsQuic after checking for QUIC support to free resources (#75521) #80623

Closed
StephenBonikowsky opened this issue Jan 13, 2023 · 8 comments
Assignees
Labels
area-System.Net.Quic partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@StephenBonikowsky
Copy link
Member

StephenBonikowsky commented Jan 13, 2023

Description

Manual backport of #75521 (which is in turn a backport of #75163 and #75441)
Fixes #74629
Replaces #75330

cc: @wfurt

Customer Impact

Memory (and number of processes) regression in .NET 7.0 from .NET 6.0 for all applications (on Win11+/Win2022+ and Linux with OpenSsl 1.1):
Even if app is not using HTTP/3 (or QUIC), we initialize early on native MsQuic.dll which always allocates threads (2* number of logical cores). Thus, causes unnecessary resource increase even if the process does not end up using HTTP/3 at all (HTTP/3 is opt-in like HTTP/2).

The fix closes all MsQuic resources after their initialization.
The change builds on fix in msquic 2.1.1 (Helix images were updated by PR #75479).

Affected platforms include Windows 11, Windows Server 2022, many Linux platforms with msquic package installed.

Note: The same problem exists also in 6.0, but hidden under opt-in switch into HTTP/3 support. We had 1 customer with ~50-core machine who noticed that in a dump and were surprised to see so many extra threads they didn't know about / they didn't need. At minimum it will help avoid developer confusion in dump investigations in such cases.

Testing

Functional tests suite passes as part of the CI, resource consumption was checked manually.

Risk

Low, the fix consists of gracefully de-initializing MsQuic library in the process after checking QUIC support. The library is re-initialized only when actually needed.

@StephenBonikowsky StephenBonikowsky added Servicing-consider Issue for next servicing release review partner-impact This issue impacts a partner who needs to be kept updated labels Jan 13, 2023
@StephenBonikowsky StephenBonikowsky added this to the 6.0.x milestone Jan 13, 2023
@ghost
Copy link

ghost commented Jan 13, 2023

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

Issue Details

Description

Manual backport of #75521 (which is in turn a backport of #75163 and #75441)
Fixes #74629
Replaces #75330

cc: @wfurt

Customer Impact

Memory (and number of processes) regression in .NET 7.0 from .NET 6.0 for all applications (on Win11+/Win2022+ and Linux with OpenSsl 1.1):
Even if app is not using HTTP/3 (or QUIC), we initialize early on native MsQuic.dll which always allocates threads (2* number of logical cores). Thus, causes unnecessary resource increase even if the process does not end up using HTTP/3 at all (HTTP/3 is opt-in like HTTP/2).

The fix closes all MsQuic resources after their initialization.
The change builds on fix in msquic 2.1.1 (Helix images were updated by PR #75479).

Affected platforms include Windows 11, Windows Server 2022, many Linux platforms with msquic package installed.

Note: The same problem exists also in 6.0, but hidden under opt-in switch into HTTP/3 support. We had 1 customer with ~50-core machine who noticed that in a dump and were surprised to see so many extra threads they didn't know about / they didn't need. At minimum it will help avoid developer confusion in dump investigations in such cases.

Testing

Functional tests suite passes as part of the CI, resource consumption was checked manually.

Risk

Low, the fix consists of gracefully de-initializing MsQuic library in the process after checking QUIC support. The library is re-initialized only when actually needed.

Reproduction Steps

Expected behavior

Actual behavior

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: StephenBonikowsky
Assignees: halter73, JamesNK, Tratcher
Labels:

Servicing-consider, area-System.Net.Quic, partner-impact

Milestone: 6.0.x

@teo-tsirpanis
Copy link
Contributor

Did you want to open a Pull Request? 😅

@karelz
Copy link
Member

karelz commented Jan 13, 2023

@wfurt is it feasible to backport? If yes, we should just reopen #74629 and close this issue as duplicate. Otherwise it will become messy in tracking.

@karelz karelz removed the Servicing-consider Issue for next servicing release review label Jan 13, 2023
@rzikm
Copy link
Member

rzikm commented Jan 16, 2023

@wfurt is it feasible to backport? If yes, we should just reopen #74629 and close this issue as duplicate. Otherwise it will become messy in tracking.

It can be backported, but the question is how much impact will it make. Since the additional threads are allocated only when the user opts in for HTTP3 support which implies he intends to use it. So fixing this would improve only the situation when the user enables HTTP3, but does not use it, which seems kind of niche.

BTW. Users should not be using HTTP3 in production in .NET 6 anyway, because it implements only draft versions of HTTP3 and QUIC. At the very least, talking to 3rd party servers is unlikely to work since most public implementations don't support draft versions anymore.

@wfurt
Copy link
Member

wfurt commented Jan 16, 2023

Both Quic & H3 are preview and pretty rough in 6.0 IMHO. We should encourage users to migrate...

@halter73
Copy link
Member

@rzikm The problem is that Kestrel is always checks whether QUIC is supported in release/6.0 whether or not HTTP/3 is actually used.

https://github.com/dotnet/aspnetcore/blob/1763467671d64c7d6316ac3874036494fe1fff47/src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs#L30-L34

https://github.com/dotnet/aspnetcore/blob/1763467671d64c7d6316ac3874036494fe1fff47/src/Servers/Kestrel/Transport.Quic/src/WebHostBuilderQuicExtensions.cs#L18-L30

The theory behind why the suppression was okay is that HttpProtocols enum has [RequiresPreviewFeatures] on its Http3 values which prevents the transport from actually being used

https://github.com/dotnet/aspnetcore/blob/1763467671d64c7d6316ac3874036494fe1fff47/src/Servers/Kestrel/Core/src/HttpProtocols.cs#L19-L22

Unfortunately, due to layering, it's not easy to prevent Kestrel from checking whether quic is supported when HTTP/3 is disabled. That's because HTTP/3 could be enabled after all the services have been added, and Kestrel uses the presence of the IMultiplexedConnectionListenerFactory to determine whether QUIC is supported at all in the HttpProtocols.Http1AndHttp2AndHttp3 case. See https://github.com/dotnet/aspnetcore/pull/44755/files#r1006327156 for the original conversation about this. @JamesNK

Fundamentally, even if it would be best if Kestrel could avoid checking it more, Kestrel will always need to check IsSupported sometimes, and this shouldn't spawn threads that never get closed.

@rzikm
Copy link
Member

rzikm commented Jan 18, 2023

I see, then backporting makes sense.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 18, 2023
@ManickaP ManickaP assigned ManickaP and unassigned wfurt Jan 19, 2023
@karelz
Copy link
Member

karelz commented Jan 20, 2023

Duplicate of #74629

@karelz karelz marked this as a duplicate of #74629 Jan 20, 2023
@karelz karelz closed this as completed Jan 20, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2023
@teo-tsirpanis teo-tsirpanis closed this as not planned Won't fix, can't repro, duplicate, stale Jan 20, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

No branches or pull requests

9 participants