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 #80785

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Jan 18, 2023

Manual backport of #75521 in release/7.0 (which is in turn a backport of #75163 and #75441 in main)
Fixes #74629

Customer Impact

Memory (and number of threads) regression in Kestrel .NET 6.0 against .NET 5.0 for all applications (on Win11+/Win2022+ and Linux with OpenSsl 1.1):
Even if app is not using HTTP/3 (or QUIC), Kestrel initializes native MsQuic.dll early on, 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).
Note: HttpClient does not have this problem on .NET 6.0, only Kestrel does. HttpClient regression was first in .NET 7.0 - which was addressed by PR #75521.

We had 1 customer with ~50-core machine who noticed the problem 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.

The fix closes all MsQuic resources after their initialization.

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

Testing

  • Manual - local testing with and without the change, also tested Linux version with different msquic versions installed on the machine
  • CI

Risk

Low, the fix consists of gracefully de-initializing MsQuic library in the process after checking QUIC support. It is backport of fix from 7.0 GA - conceptually the changes are doing the same, but the code is different between 6.0 and 7.0.
The library is re-initialized only when actually needed.

@ghost
Copy link

ghost commented Jan 18, 2023

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

Issue Details

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

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@ManickaP ManickaP marked this pull request as ready for review January 19, 2023 15:10
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@karelz karelz added this to the 6.0.x milestone Jan 20, 2023
@ManickaP ManickaP added the Servicing-consider Issue for next servicing release review label Jan 20, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 24, 2023
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.15 Jan 24, 2023
@ManickaP
Copy link
Member Author

Staging pipeline errors are: #80628
Runtime pipeline timeouts are: #76454

@carlossanlop
Copy link
Member

Approved by Tactics for 6.0.15.
Signed-off by area owners.
No OOB package changes needed for System.Net.Quic.
CI failures investigated as unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit a58e25a into dotnet:release/6.0 Feb 8, 2023
@ManickaP ManickaP deleted the release/6.0 branch February 8, 2023 10:05
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants