-
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
Make NativeLibrary Load/TryLoad use ALC extension points for the specified assembly #34519
Make NativeLibrary Load/TryLoad use ALC extension points for the specified assembly #34519
Conversation
logic of DllImport when called in that assembly Call extension points: DllImportResolver delegate, LoadUnmanagedDll function, ResolvingUnmanagedDll event. Avoid calling if already in an explicit NativeLibrary load scenario.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs
Outdated
Show resolved
Hide resolved
src/coreclr/src/vm/dllimport.cpp
Outdated
class UseExtensionPoints | ||
{ | ||
public: | ||
UseExtensionPoints() |
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 that this recursion block should rather be the AssemblyLoadContext - to disallow recursite call on the same AssemblyContext and the same name.
Also, if you block it in C#, it will work for both CoreCLR and Mono, no need to duplicate it.
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 was trying to find where I could just have it in one place, so I had put it here since the DllImportResolver
delegate is not tied to the ALC.
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.
Although NativeLibrary.LoadLibraryCallbackStub
is already shared and can get at the ALC.
If the tracking goes on in the AssemblyLoadContext
, I'd need to add some shared part that conditionally calls LoadUnmanagedDll
based on the tracking - then that tracking could cover all three extension points.
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 am not sure whether we want to make this fix to touch DllImportResolver. It is not required to address the problem described in the issue, and I can only think about problems that it would introduce.
Would it make sense to do this fix for the ALC only, and keep DllImportResolver on the existing plan (ie keep calling it for DllImports only and not for NativeLibrary.Load)?
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 it would make the API behaviour a bit less intuitive/obvious to me? Saying this API that takes an Assembly
behaves as if DllImport
was used in the given Assembly
is an easy line to draw. But saying this API that takes an Assembly
will not use the per-assembly callback for the given Assembly
, but will use the extension points of the AssemblyLoadContext
in which the given Assembly
is loaded becomes less clear-cut.
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 am looking at this from the intended use of the APIs:
-
DllImportResolver
enables the assembly to resolve native libraries for its own DllImports. It is expected to be overriden by the assembly for itself only, and you can have just one of these registered per assembly. I cannot think about a scenario where it would be desirable for the DllImportResolver handler to be called forNativeLibrary.Load
calls. If the assembly wants to call its own DllImportResolver handler, it can just do it directly. -
AssemblyLoadContext
enables resolving native libraries for all assemblies loaded into it, without the assemblies itself being aware of it.
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 the starting premise is that the NativeLoad.Load
that takes an Assembly
should behave as if DllImport
was used in the given assembly, shouldn't it do exactly that? Regardless of the expected use of the other APIs that can modify the behaviour for that assembly?
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.
NativeLoad.Load
that takes an Assembly should behave as if DllImport was used in the given assembly
That may have been over-simplification.
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.
Keeping the DllImportResolver as is also reduces breaking potential of the fix. E.g. somebody may have DllImportResolver that starts with:
if (name != "mydll")
throw new Exception();
src/coreclr/src/vm/dllimport.cpp
Outdated
|
||
// This thread local and UseExtensionPoints class are used to guard against infinite recursion. | ||
// The managed APIs (NativeLibarary.Load/TryLoad) that invoke managed extension points can also | ||
// be called from within the extension points themselves. This class tracks whether or not the |
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 the call has different argument, then we should allow. There are valid cases where somebody may want to do that.
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.
BTW: We also have an option to "do nothing" and see whether we hear any reports of breaks due to infinite recursion.
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 the 'do nothing' option becomes much more appealing if we avoid calling into the DllImportResolver
(since that seems like the more likely to have breaks based on usage/searching around). Especially with differentiating the arguments, it seems like the extra tracking may not be worth doing without knowing it will be an issue (and easy enough to add if there is).
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.
Removed the tracking/blocking and only going through the extension points tied to the ALC now.
Make the
NativeLibrary.Load/TryLoad
overloads that take anAssembly
mimic the logic of native assembly resolution for theAssemblyLoadContext
of the specified assembly. They will go through the extension points for theAssemblyLoadContext
of the specified assembly:LoadUnmanagedDll
functionResolvingUnmanagedDll
eventThey will no go through the extension points that are not tied to the
AssemblyLoadContext
:DllImportResolver
delegate)Fix #13819