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/7.0] Close MsQuic after checking for QUIC support to free resources (#75163, #75441) #75521

Merged
merged 4 commits into from
Sep 23, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Sep 13, 2022

Manual 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.

rzikm and others added 2 commits September 13, 2022 13:20
…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 ghost assigned rzikm Sep 13, 2022
@rzikm rzikm added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 13, 2022
@rzikm
Copy link
Member Author

rzikm commented Sep 13, 2022

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 rzikm requested a review from a team September 13, 2022 11:30
@rzikm
Copy link
Member Author

rzikm commented Sep 13, 2022

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

@karelz
Copy link
Member

karelz commented Sep 13, 2022

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.

@karelz karelz added this to the 7.0.0 milestone Sep 13, 2022
@rzikm 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
@rzikm
Copy link
Member Author

rzikm commented Sep 22, 2022

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.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: s_api

return;
}

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

Choose a reason for hiding this comment

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

const int ArraySize = 4;

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

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

Choose a reason for hiding this comment

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

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.

return;
}

MsQuicOpenVersion = (delegate* unmanaged[Cdecl]<uint, QUIC_API_TABLE**, int>)NativeLibrary.GetExport(msQuicHandle, nameof(MsQuicOpenVersion));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

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

@rzikm
Copy link
Member Author

rzikm commented Sep 23, 2022

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
Copy link
Member Author

rzikm commented Sep 23, 2022

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
Copy link
Member

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

@halter73
Copy link
Member

/backport to release/6.0

@github-actions github-actions bot unlocked this conversation Jan 18, 2023
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

@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
Copy link
Contributor

@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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants