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

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

Manual backport of #75163 and #75441
Fixes #74629
Replaces #75330

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.

…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>

rzikm · 2022-09-13T11:29:36Z

Adding no merge as it was present on previous (closed) PR, @karelz up to you if we want to remove it based on #75330 (comment), or keep it to let the change gain more bake time in main before merging.

rzikm · 2022-09-13T12:16:17Z

I removed updates to Helix Queues, as they will be done by #75479

karelz · 2022-09-13T13:17:56Z

We should discuss it in today's meeting. Especially in the light of #75493 I am hesitant to push any unloading into 7.0 at this moment.

rzikm · 2022-09-22T06:21:11Z

Since #75493 has been closed (due to not occurring anymore on main), most likely as a result of #75441 (included in this PR), we think there has been enough bake time to confidently merge this in 7.0.0.

stephentoub · 2022-09-22T14:18:11Z

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

@@ -47,7 +51,8 @@ private MsQuicApi(QUIC_API_TABLE* apiTable)
        }
    }

-    internal static MsQuicApi Api { get; } = null!;
+    private static readonly Lazy<MsQuicApi> _api = new Lazy<MsQuicApi>(AllocateMsQuicApi);


stephentoub · 2022-09-22T14:20:54Z

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

            return;
        }

        try
        {
-            if (!NativeLibrary.TryGetExport(msQuicHandle, "MsQuicOpenVersion", out IntPtr msQuicOpenVersionAddress))
+            // Check version
+            int arraySize = 4;


const int ArraySize = 4;

stephentoub · 2022-09-22T14:23:11Z

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

@@ -19,6 +20,9 @@ internal sealed unsafe partial class MsQuicApi

    private static readonly Version MsQuicVersion = new Version(2, 1);


I got confused by this naming. This isn't the version of msquic that's being used, but rather than minimum supported version, yes? It should probably be named accordingly, e.g. MinimumSupportedMsQuicVersion.

CarnaViire · 2022-09-22T15:28:25Z

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

+            return;
+        }
+
+        MsQuicOpenVersion = (delegate* unmanaged[Cdecl]<uint, QUIC_API_TABLE**, int>)NativeLibrary.GetExport(msQuicHandle, nameof(MsQuicOpenVersion));


Old version had TryGetExport for these methods, but what will happen now if the export fails? Will it throw? Will it return null?

NativeLibrary.GetExport will throw an exception when the export is not found in the library.

From #75441 (comment) : The failures to get exports from the MsQuic library should be fatal. If we fail to get the export, it means that there is something very wrong.

ManickaP

LGTM.
I assume that all public APIs make sure Api instance is not accessed in case IsSupported is not true.

rzikm · 2022-09-23T10:10:01Z

LGTM. I assume that all public APIs make sure Api instance is not accessed in case IsSupported is not true.

Yes, Both QuicListener.ListenAsync and QuicConnection.ConnectAsync check the property. I don't think there is any other public API that needs to be guarded.

rzikm · 2022-09-23T10:11:38Z

CI Failure looks unrelated:

System.Net.Http.Functional.Tests.SyncHttpHandler_HttpClientHandler_Cancellation_Test.GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly(chunkedTransfer: True, connectionClose: False, readOrCopyToAsync: False) [FAIL]
      System.TimeoutException : The operation has timed out.
      Stack Trace:
        /_/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs(260,0): at System.Net.Http.Functional.Tests.HttpClientHandler_Cancellation_Test.<>c__DisplayClass4_2.<<GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly>b__0>d.MoveNext()
        --- End of stack trace from previous location ---
        /_/src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs(101,0): at System.Net.Test.Common.LoopbackServer.CreateServerAsync(Func`2 funcAsync, Options options)
        /_/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs(228,0): at System.Net.Http.Functional.Tests.HttpClientHandler_Cancellation_Test.GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly(Boolean chunkedTransfer, Boolean connectionClose, Boolean readOrCopyToAsync)

I will restart the affected leg

carlossanlop · 2022-09-23T16:08:12Z

CI passed after the re-run. Approved by Tactics. Signed off. Ready to merge.

halter73 · 2023-01-18T02:35:02Z

/backport to release/6.0

github-actions · 2023-01-18T02:35:16Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/3945172799

github-actions · 2023-01-18T02:37:23Z

@halter73 backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Unload MsQuic after checking for QUIC support to free resources (#75163)
Using index info to reconstruct a base tree...
M	eng/pipelines/libraries/helix-queues-setup.yml
A	src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs deleted in HEAD and modified in Unload MsQuic after checking for QUIC support to free resources (#75163). Version Unload MsQuic after checking for QUIC support to free resources (#75163) of src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs left in tree.
Auto-merging eng/pipelines/libraries/helix-queues-setup.yml
CONFLICT (content): Merge conflict in eng/pipelines/libraries/helix-queues-setup.yml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Unload MsQuic after checking for QUIC support to free resources (#75163)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-01-18T02:37:24Z

@halter73 an error occurred while backporting to release/6.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

rzikm and others added 2 commits September 13, 2022 13:20

Don't unload MsQuic from the process (dotnet#75441)

67b137f

dotnet-issue-labeler bot added the area-System.Net.Http label Sep 13, 2022

ghost assigned rzikm Sep 13, 2022

rzikm mentioned this pull request Sep 13, 2022

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

Closed

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

rzikm requested a review from a team September 13, 2022 11:30

Revert helix queues change (to be done in another PR)

d3b4084

karelz added this to the 7.0.0 milestone Sep 13, 2022

rzikm added Servicing-consider Issue for next servicing release review and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Sep 22, 2022

karelz requested review from stephentoub, ManickaP, wfurt and CarnaViire September 22, 2022 13:07

karelz approved these changes Sep 22, 2022

View reviewed changes

stephentoub reviewed Sep 22, 2022

View reviewed changes

stephentoub approved these changes Sep 22, 2022

View reviewed changes

This was referenced Sep 22, 2022

Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979

Closed

System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildInvalidSignatureTwice failure #65448

Closed

CarnaViire reviewed Sep 22, 2022

View reviewed changes

rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 22, 2022

CarnaViire approved these changes Sep 22, 2022

View reviewed changes

Code review feedback

454ce6b

ManickaP approved these changes Sep 23, 2022

View reviewed changes

carlossanlop merged commit 23c9839 into dotnet:release/7.0 Sep 23, 2022

karelz mentioned this pull request Sep 23, 2022

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

Closed

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

Tratcher mentioned this pull request Nov 2, 2022

Remove premature QuicListener.IsSupported check dotnet/aspnetcore#44755

Closed

StephenBonikowsky mentioned this pull request Jan 13, 2023

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

Closed

github-actions bot unlocked this conversation Jan 18, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rzikm commented Sep 13, 2022 •

edited by karelz

Loading

rzikm commented Sep 13, 2022 •

edited

Loading

rzikm commented Sep 13, 2022

karelz commented Sep 13, 2022

rzikm commented Sep 22, 2022

stephentoub Sep 22, 2022

stephentoub Sep 22, 2022

stephentoub Sep 22, 2022

CarnaViire Sep 22, 2022

jkotas Sep 22, 2022

ManickaP left a comment

rzikm commented Sep 23, 2022

rzikm commented Sep 23, 2022

carlossanlop commented Sep 23, 2022

halter73 commented Jan 18, 2023

github-actions bot commented Jan 18, 2023

github-actions bot commented Jan 18, 2023

github-actions bot commented Jan 18, 2023

		@@ -19,6 +20,9 @@ internal sealed unsafe partial class MsQuicApi

		private static readonly Version MsQuicVersion = new Version(2, 1);

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

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

Conversation

rzikm commented Sep 13, 2022 • edited by karelz Loading

Customer Impact

Testing

Risk

rzikm commented Sep 13, 2022 • edited Loading

rzikm commented Sep 13, 2022

karelz commented Sep 13, 2022

rzikm commented Sep 22, 2022

stephentoub Sep 22, 2022

Choose a reason for hiding this comment

stephentoub Sep 22, 2022

Choose a reason for hiding this comment

stephentoub Sep 22, 2022

Choose a reason for hiding this comment

CarnaViire Sep 22, 2022

Choose a reason for hiding this comment

jkotas Sep 22, 2022

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

rzikm commented Sep 23, 2022

rzikm commented Sep 23, 2022

carlossanlop commented Sep 23, 2022

halter73 commented Jan 18, 2023

github-actions bot commented Jan 18, 2023

github-actions bot commented Jan 18, 2023

github-actions bot commented Jan 18, 2023

rzikm commented Sep 13, 2022 •

edited by karelz

Loading

rzikm commented Sep 13, 2022 •

edited

Loading