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

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

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Sep 2, 2022

Contributes to #74952.

This reverts commit 7a45201 to unblock CI.
Reopens #74629

@ghost
Copy link

ghost commented Sep 2, 2022

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

Issue Details

This reverts commit 7a45201.

Author: rzikm
Assignees: -
Labels:

area-System.Net

Milestone: -

@rzikm rzikm requested a review from a team September 2, 2022 07:05
@rzikm
Copy link
Member Author

rzikm commented Sep 2, 2022

It turns out the test failure may be something environmental, because it is caused by MsQuic returning QUIC_STATUS_OUT_OF_MEMORY, so I might close this PR if we decide to mitigate this in other way (like temporarily disabling quic test runs on affected docker images)

@jkotas
Copy link
Member

jkotas commented Sep 2, 2022

It turns out the test failure is likely something environmental, because it is caused by MsQuic returning QUIC_STATUS_OUT_OF_MEMORY

Have you ever seen QUIC_STATUS_OUT_OF_MEMORY when initializing Quic before?

It can be very well a real problem with the change where loading/unloading/loading_again native library or starting/shutting_down/starting_again a lot of threads is hitting some pathological behaviors.

@jkotas jkotas merged commit 953f524 into dotnet:main Sep 2, 2022
@rzikm
Copy link
Member Author

rzikm commented Sep 2, 2022

Have you ever seen QUIC_STATUS_OUT_OF_MEMORY when initializing Quic before?

No.

I find it quite weird, doing the unload/load dance immediately works well, but when we load+unload as part of QUIC support checking and re-load and reopen the library again later, it failed deterministically on the machine I was testing it. I will try to see where the status comes from in MsQuic, do you perhaps have some other ideas how to investigate further?

@jkotas
Copy link
Member

jkotas commented Sep 2, 2022

If you have a machine where it failed deterministically, it would be the place to start.

My guess is that it will be a bug in msquic library unloading. Library unloading is typically buggy and it does not work for most libraries out there. We may want to keep msquic loaded once it gets loaded (ie do not ever call NativeLibrary.Free on it). The resource hit from keeping the library loaded should be fairly small. You can still close the Apis to shutdown the msquic threadpool.

@karelz karelz added this to the 8.0.0 milestone Sep 3, 2022
wfurt added a commit that referenced this pull request Sep 6, 2022
wfurt added a commit that referenced this pull request Sep 8, 2022
* 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>
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
@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to 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.

3 participants