Skip to content

Unload MsQuic after checking for QUIC support to free resources - #75163

Merged
wfurt merged 4 commits into
mainfrom
revert-74984-73871-revert
Sep 8, 2022
Merged

Unload MsQuic after checking for QUIC support to free resources#75163
wfurt merged 4 commits into
mainfrom
revert-74984-73871-revert

Conversation

@wfurt

@wfurtwfurt commented Sep 6, 2022

Copy link
Copy Markdown
Member

Fixes#74629

This PR unloads MsQuic library from the process after we check whether it is supported. This saves some resources when HTTP/3 is not used (e.g. the server does not advertise HTTP/3)

Test app:

usingSystem.Net.Http;HttpClientclient=newHttpClient();awaitclient.GetAsync("https://www.microsoft.com");System.Console.WriteLine("Done");System.Console.ReadLine();

Results on Windows11

DiffBeforeAfter
Threads6223
PrivateMemorySize1338572812083200

Brings back PR #74749, reverted by PR #74984

@ghostghost added the area-System.Net label Sep 6, 2022
@ghostghost assigned wfurtSep 6, 2022
@ghost

ghost commented Sep 6, 2022

Copy link
Copy Markdown

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

Issue Details

Brings back #74749, reverted by #74984

Author:wfurt
Assignees:-
Labels:

area-System.Net

Milestone:-

@ghost

ghost commented Sep 6, 2022

Copy link
Copy Markdown

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

Issue Details

Brings back #74749, reverted by #74984

Author:wfurt
Assignees:wfurt
Labels:

area-System.Net, area-System.Net.Quic

Milestone:-

@wfurt
wfurt requested a review from rzikmSeptember 7, 2022 01:46
@wfurt
wfurt marked this pull request as ready for review September 7, 2022 01:46
@wfurt

wfurt commented Sep 7, 2022

Copy link
Copy Markdown
MemberAuthor

Debian 10 on ARM64 is OK
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-75163-merge-ee93b0bfb1f4439da8/System.Net.Quic.Functional.Tests/1/console.99482cb3.log?helixlogtype=result

Console log: 'System.Net.Quic.Functional.Tests' from job ee93b0bf-b1f4-439d-a82a-e3bbd340118b (ubuntu.1804.armarch.open) using docker image mcr.microsoft.com/dotnet-buildtools/prereqs:debian-10-helix-arm64v8-20220906200500-06f234f on ddvsotx2l103
/root/helix/work/workitem/e /root/helix/work/workitem/e
Discovering: System.Net.Quic.Functional.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Net.Quic.Functional.Tests (found 106 of 112 test cases)
Starting: System.Net.Quic.Functional.Tests (parallel test collections = on, max threads = 6)
System.Net.Quic.Tests.MsQuicPlatformDetectionTests.UnsupportedPlatforms_ThrowsPlatformNotSupportedException [SKIP]
Condition(s) not met: "IsQuicUnsupported"
Finished: System.Net.Quic.Functional.Tests
=== TEST EXECUTION SUMMARY ===
System.Net.Quic.Functional.Tests Total: 326, Errors: 0, Failed: 0, Skipped: 1, Time: 124.431s

@rzikm

rzikm commented Sep 7, 2022

Copy link
Copy Markdown
Member

Would you mind picking changes from #74985 here as well?

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@karelzkarelz changed the title Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)""Unload MsQuic after checking for QUIC support to free resourcesSep 8, 2022
@karelzkarelz added this to the 8.0.0 milestone Sep 8, 2022
@karelz

karelz commented Sep 8, 2022

Copy link
Copy Markdown
Member

@wfurt I assume your Debian 10 ARM64 validation above was with msquic 2.1.1 package. Can you please confirm?

@karelz

Copy link
Copy Markdown
Member

Also, should we mark the PR as blocked until we finish updating msquic 2.1.1 properly? (incl. Windows source code snapshot)

@wfurt

wfurt commented Sep 8, 2022

Copy link
Copy Markdown
MemberAuthor

no, that is still floating in. It has Docker image based on dotnet/msquic main e.g. has startup changes but it is not the official bits.

@wfurt
wfurt merged commit e995620 into mainSep 8, 2022
@wfurt
wfurt deleted the revert-74984-73871-revert branch September 8, 2022 16:48
@rzikm

rzikm commented Sep 9, 2022

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3020723954

@github-actions

Copy link
Copy Markdown
Contributor

@rzikm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)" (#74984)"
Applying: update helix images
Using index info to reconstruct a base tree...
M	eng/pipelines/libraries/helix-queues-setup.yml
Falling back to patching base and 3-way merge...
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 0002 update helix images
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!

rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Sep 9, 2022
…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 added a commit to rzikm/dotnet-runtime that referenced this pull request Sep 13, 2022
…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>
carlossanlop pushed a commit that referenced this pull request Sep 23, 2022
…sources (#75163, #75441) (#75521)
* Unload MsQuic after checking for QUIC support to free resources (#75163)
* Revert "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>
* Don't unload MsQuic from the process (#75441)
* Revert helix queues change (to be done in another PR)
* Code review feedback
@ghostghost locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants

@wfurt@rzikm@karelz