[release/7.0] Unload MsQuic after checking for QUIC support to free resources (#75163) #75330

rzikm · 2022-09-09T08:05:37Z

Manual backport of #75163
Fixes #74629

Customer Impact

When using HttpClient, we also check whether the running platform supports QUIC (to enable HTTP/3). However, the way we are checking QUIC support causes many threads to be allocated in the native MsQuic library (2* number of logical cores). This causes unnecessary resource increase even if the process does not end up using HTTP/3 at all (HTTP/3 is opt-in). This would therefore cause regression in memory usage when upgrading to .NET 7 in such cases.

The change builds on fix in msquic -- therefore it includes also update of Docker images to version with the right msquic version (2.1.1).

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

Note: The same problem exists also in 6.0 and we had 1 customer with ~50-core machine who noticed that and were surprised to see so many extra threads they didn't know about / they didn't need. At minimum it will help avoid confusion 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 unloading MsQuic library from the process after checking QUIC support. The library is reloaded only when actually needed.

…et#75163) * Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (dotnet#74749)" (dotnet#74984)" This reverts commit 953f524. * update helix images * update helix images * Improve diagnostics when opening MsQuic Co-authored-by: Radek Zikmund <radekzikmund@microsoft.com>

ghost · 2022-09-09T08:06:42Z

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

Issue Details

Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (Unload MsQuic after checking for QUIC support to free resources. #74749)" (Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)" #74984)"

This reverts commit 953f524.

update helix images
update helix images
Improve diagnostics when opening MsQuic

Co-authored-by: Radek Zikmund radekzikmund@microsoft.com

Fixes Issue

main PR

Description

Customer Impact

Regression

Testing

Risk

Package authoring signed off?

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author:	rzikm
Assignees:	rzikm
Labels:	`area-System.Reflection.Metadata`, `new-api-needs-documentation`
Milestone:	-

ghost · 2022-09-09T08:07:32Z

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

Issue Details

Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (Unload MsQuic after checking for QUIC support to free resources. #74749)" (Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)" #74984)"

This reverts commit 953f524.

update helix images
update helix images
Improve diagnostics when opening MsQuic

Co-authored-by: Radek Zikmund radekzikmund@microsoft.com

Fixes Issue

main PR

Description

Customer Impact

Regression

Testing

Risk

Package authoring signed off?

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author:	rzikm
Assignees:	rzikm
Labels:	`area-System.Net.Quic`
Milestone:	-

danmoseley · 2022-09-09T15:38:06Z

Approved -- significant customer impact, new feature, and hitting existing customers that don't use it. Would certainly service for it.

But, more eyes would be good at this point. @stephentoub would it be possible for you to review before we merge here?

stephentoub · 2022-09-09T15:42:15Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

        }
        finally
        {
-            if (!IsQuicSupported)
+            // Unload the library, we will load it again when we actually use QUIC
+            NativeLibrary.Free(msQuicHandle);


I thought @jkotas had advised against actually unloading it. Did we decide it makes sense to anyway?

Also note that this is not unloading it completely. The library for msquic tracing support stays around: #74710 (comment) .

I think we can keep the same benefit without unloading the library from the process, we just need to save the IntPtr handle to a static field. The threads should all get deallocated by MsQuicClose. The dangling libmsquic.lttng.so is something I have not foreseen when writing this change.

stephentoub · 2022-09-09T15:46:19Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

+            return new MsQuicApi(apiTable);
+        }
+
+        ThrowHelper.ThrowIfMsQuicError(openStatus);


throw ThrowHelper.GetExceptionForMsQuicStatus(openStatus);

and then remove the next line?

karelz · 2022-09-09T16:56:58Z

Yeah, I have suspicion that the mysterious memory corruption we are chasing may be caused by unloading msquic as @jkotas suggested above. Too high risk for not-so-large value. Let's hold off merging this PR. We will likely not take it into 7.0 at all.

danmoseley · 2022-09-09T22:43:58Z

Closing per comments above.

karelz · 2022-09-12T14:44:36Z

FYI: The mysterious memory corruption was chased down to be unrelated problem in LTTNG triggered in our test environment - see #74795 for a test-only fix.

We have CR feedback addressed in #75441 -- calling MsQuicClose without unloading msquic DLL.

Given the churn in the area, I would like to see a little bit more bake time in main for the final changes. Once we have confidence it didn't break anything and is not triggering problems in LTTNG or elsewhere, I would be fine bringing it into 7.0. That will be already too late for GA, so we will leave it for servicing request if it confuses customers enough and if/when we get complains from customers.
@stephentoub @jkotas would you suggest something else?

stephentoub · 2022-09-12T15:27:35Z

I would like to see this fixed for .NET 7; it is a regression from .NET 6 (I just tried with .NET 6 on Win11, and without setting the experimental HTTP/3 flag and just making a simple HttpClient request, msquic is never loaded and the extra threads aren't created) that pretty much every app is going to end up paying even if HTTP/3 is never used. I think it's fine to hold off for a few days or however long to ensure this actually addresses the problem without resulting in new instabilities, and it can be fixed post-RC2 for 7.0.0 or in servicing for 7.0.1.

karelz · 2022-09-12T15:30:16Z

Agreed in offline discussion. @rzikm feel free to reopen the PR and merge your PR-feedback. We will mark it NO-MERGE and decide how long we want to confirm it didn't cause any regression in main.

stephentoub · 2022-09-12T15:36:16Z

That said, can we take another look at whether we can do more on the SocketsHttpHandler side to even avoid touching quic at all unless there's a demonstrated need? e.g. with the default version of a request being HTTP/1.1 and the default version policy of RequestVersionOrLower, couldn't we avoid initializing HTTP/3 support at all unless someone makes a request with either a different policy or explicitly requesting HTTP/3?

jkotas · 2022-09-12T17:13:15Z

We will mark it NO-MERGE and decide how long we want to confirm it didn't cause any regression in main.

My preference would be to push the Quic fixes to release/7.0 sooner rather than later (e.g. after one day of bake time). Main is not getting a lot of scrutiny outside our CI system currently, so you are unlikely to get a lot more extra signal by waiting longer.

rzikm · 2022-09-13T11:27:21Z

I was unable to reopen this PR, so I created new at #75521

rzikm requested review from radical, lewing, pavelsavara, thaystg, vargaz, lambdageek, BrzVlad, naricc, SamMonoRT, marek-safar and MichalStrehovsky as code owners September 9, 2022 08:05

ghost assigned rzikm Sep 9, 2022

rzikm changed the base branch from main to release/7.0 September 9, 2022 08:06

dotnet-issue-labeler bot added area-System.Reflection.Metadata new-api-needs-documentation labels Sep 9, 2022

rzikm removed request for radical, lewing, vargaz, marek-safar, pavelsavara, lambdageek, thaystg, BrzVlad, MichalStrehovsky, naricc and SamMonoRT September 9, 2022 08:06

rzikm added area-System.Net.Quic and removed area-System.Reflection.Metadata new-api-needs-documentation labels Sep 9, 2022

dotnet deleted a comment from dotnet-issue-labeler bot Sep 9, 2022

rzikm requested a review from a team September 9, 2022 08:08

karelz added this to the 7.0.0 milestone Sep 9, 2022

karelz approved these changes Sep 9, 2022

View reviewed changes

stephentoub reviewed Sep 9, 2022

View reviewed changes

karelz added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 9, 2022

danmoseley closed this Sep 9, 2022

rzikm deleted the 75163-release-7.0 branch September 12, 2022 15:08

karelz mentioned this pull request Sep 12, 2022

HttpClient creates Quic threads even if HTTP/3 is not in use #74629

Closed

rzikm mentioned this pull request Sep 13, 2022

[release/7.0] Close MsQuic after checking for QUIC support to free resources (#75163, #75441) #75521

Merged

ghost locked as resolved and limited conversation to collaborators Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/7.0] Unload MsQuic after checking for QUIC support to free resources (#75163) #75330

[release/7.0] Unload MsQuic after checking for QUIC support to free resources (#75163) #75330

rzikm commented Sep 9, 2022 •

edited by karelz

Loading

ghost commented Sep 9, 2022

Description

Customer Impact

Regression

Testing

Risk

Package authoring signed off?

ghost commented Sep 9, 2022

Description

Customer Impact

Regression

Testing

Risk

Package authoring signed off?

danmoseley commented Sep 9, 2022

stephentoub Sep 9, 2022

jkotas Sep 9, 2022

rzikm Sep 9, 2022 •

edited

Loading

stephentoub Sep 9, 2022 •

edited

Loading

karelz commented Sep 9, 2022 •

edited

Loading

danmoseley commented Sep 9, 2022

karelz commented Sep 12, 2022

stephentoub commented Sep 12, 2022 •

edited

Loading

karelz commented Sep 12, 2022

stephentoub commented Sep 12, 2022

jkotas commented Sep 12, 2022

rzikm commented Sep 13, 2022

[release/7.0] Unload MsQuic after checking for QUIC support to free resources (#75163) #75330

[release/7.0] Unload MsQuic after checking for QUIC support to free resources (#75163) #75330

Conversation

rzikm commented Sep 9, 2022 • edited by karelz Loading

Customer Impact

Testing

Risk

ghost commented Sep 9, 2022

Description

Customer Impact

Regression

Testing

Risk

Package authoring signed off?

ghost commented Sep 9, 2022

Description

Customer Impact

Regression

Testing

Risk

Package authoring signed off?

danmoseley commented Sep 9, 2022

stephentoub Sep 9, 2022

Choose a reason for hiding this comment

jkotas Sep 9, 2022

Choose a reason for hiding this comment

rzikm Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

stephentoub Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

karelz commented Sep 9, 2022 • edited Loading

danmoseley commented Sep 9, 2022

karelz commented Sep 12, 2022

stephentoub commented Sep 12, 2022 • edited Loading

karelz commented Sep 12, 2022

stephentoub commented Sep 12, 2022

jkotas commented Sep 12, 2022

rzikm commented Sep 13, 2022

rzikm commented Sep 9, 2022 •

edited by karelz

Loading

rzikm Sep 9, 2022 •

edited

Loading

stephentoub Sep 9, 2022 •

edited

Loading

karelz commented Sep 9, 2022 •

edited

Loading

stephentoub commented Sep 12, 2022 •

edited

Loading