-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Don't unload MsQuic from the process #75441
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsSince MsQuic library loads libmsquic.lttng.so on Linuxes it is weird to unload just the library and not the dependent libraries as well, and since we cannot touch libmsquic.lttng.so from .NET, it seems better to keep libmsquic loaded in memory to prevent any weird behavior by having the library only partially loaded. Related:
cc: @stepehtoub, @jkotas
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, small comment to consider.
private static bool TryLoadMsQuic(out IntPtr msQuicHandle) => | ||
NativeLibrary.TryLoad($"{Interop.Libraries.MsQuic}.{MsQuicVersion.Major}", typeof(MsQuicApi).Assembly, DllImportSearchPath.AssemblyDirectory, out msQuicHandle) || | ||
NativeLibrary.TryLoad(Interop.Libraries.MsQuic, typeof(MsQuicApi).Assembly, DllImportSearchPath.AssemblyDirectory, out msQuicHandle); | ||
private static IntPtr TryLoadMsQuic() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this still be named Try...
? Since it doesn't return bool and out IntPtr anymore.
@@ -20,6 +20,8 @@ internal sealed unsafe partial class MsQuicApi | |||
|
|||
private static readonly Version MsQuicVersion = new Version(2, 1); | |||
|
|||
private static readonly IntPtr MsQuicHandle = TryLoadMsQuic(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to keep the original shape of TryLoadMsQuic, this could be done as: TryLoadMsQuic(out IntPtr handle) ? handle : IntPtr.Zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, that could work, I originally wanted to set it from the static ctor using the old signature and compiler complained with Initialize all static fields in 'MsQuicApi' when those fields are declared and remove the explicit static constructor
apiTable = null; | ||
if (!NativeLibrary.TryGetExport(msQuicHandle, "MsQuicOpenVersion", out IntPtr msQuicOpenVersionAddress)) | ||
if (!NativeLibrary.TryGetExport(MsQuicHandle, "MsQuicOpenVersion", out IntPtr msQuicOpenVersionAddress)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be NativeLibrary.GetExport
.
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.
if (MsQuicHandle == IntPtr.Zero) | ||
{ | ||
// MsQuic library not loaded | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (MsQuicHandle == IntPtr.Zero) | |
{ | |
// MsQuic library not loaded | |
return; | |
} | |
if (!TryLoadMsQuic(out MsQuicHandle)) | |
{ | |
// MsQuic library not loaded | |
return; | |
} |
and delete inline initialization of MsQuicHandle above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of storing MsQuicHandle, you can consider storing the pointers to the two exports in statics instead. It would save a bit of redundant work on the re-initialization.
@@ -185,9 +176,11 @@ private static bool TryOpenMsQuic(IntPtr msQuicHandle, out QUIC_API_TABLE* apiTa | |||
return true; | |||
} | |||
|
|||
private static bool TryCloseMsQuic(IntPtr msQuicHandle, QUIC_API_TABLE* apiTable) | |||
private static bool TryCloseMsQuic(QUIC_API_TABLE* apiTable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can return void
if you incorporate my other feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…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
Since MsQuic library loads libmsquic.lttng.so on Linuxes it is weird to unload just the library and not the dependent libraries as well, and since we cannot touch libmsquic.lttng.so from .NET, it seems better to keep libmsquic loaded in memory to prevent any weird behavior by having the library only partially loaded.
Note that this change does not negate the benefit of #74749, the threads are still being stopped and deallocated in the
MsQuicClose
call.Related:
Fixes #74629
cc: @stephentoub, @jkotas