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

NativeLibrary.Load(string, Assembly, DllImportSearchPath?) should honor assembly's ALC #13819

Closed
albahari opened this issue Nov 23, 2019 · 4 comments · Fixed by #34519
Closed

Comments

@albahari
Copy link

The following overload of NativeLibrary.Load:

NativeLibrary.Load(string libraryName, Assembly assembly, DllImportSearchPath? searchPath)

should invoke the LoadUnmanagedDll method and ResolvingUnmanagedDll event of the assembly's ALC, in keeping with the API's design intention:

  • NativeLibrary.Load(string) is a low-level API that wraps the underlying library load OS API and nothing else. It is meant to be used e.g. for loading OS and similar libraries that are outside the application.
  • NativeLibrary.Load(string libraryName, Assembly assembly, DllImportSearchPath? searchPath) is a higher level API. The intention for this API was to behave as if DllImport was used in the given assembly.

Right now, it does not behave as such and ignores the assembly's ALC. Consequently, any assembly calling this method will almost certainly break unless that assembly is loaded into the default ALC. An example is Microsoft.Data.Sqlite, which calls SQLitePCLRaw.nativelibrary. This breaks right now when loaded into a non-default ALC (or when loaded dynamically).

The other overload - NativeLibrary.Load(string libraryPath) - should remain as-is.

See the second/third comment in #13472 for a full discussion.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@elinor-fung
Copy link
Member

From chatting with @swaroop-sridhar, the NativeLibrary.Load(string libraryName, Assembly assembly, DllImportSearchPath? searchPath) API was also originally intended to work at a lower level than the different managed hooks available for changing how an unmanaged library is loaded. That way, it could be used as a building block for the implementations of those extension points (DllImportResolver delegate, AssemblyLoadContext.LoadUnmanagedDll function, and AssemblyLoadContext.ResolvingUnmanagedDll event handler).

If we were to change that overload of 'NativeLibrary.Load` to actually call those extension points themselves:

  • that building block would no longer be available (not sure if there would be a simple or straightforward way to achieve the same thing anymore)
  • it could possibly be breaking for any implementations of those extension points already using it

@jkotas @vitek-karas Thoughts?

@jkotas
Copy link
Member

jkotas commented Apr 1, 2020

Do we know about any existing AssemblyLoadContext implementation that would hit the problems you have listed?

A few thoughts:

  • If NativeLibrary.Load does not call into AssemblyLoadContext logic, it is hard to workaround it. The problem described in this issue feels like a bug that should be fixed.
  • If the recursive calls from AssemblyLoadContext turned out to be a compat problem, we can mitigate them by having thread-static in AssemblyLoadContext that breaks the infinitive recursion (not pretty but should work).

@elinor-fung
Copy link
Member

I don't know of existing implementations that would hit the compatibility problems. I see assorted implementations of DllImportResolver and LoadUnmanagedDll using that NativeLibrary.Load overload, but the logic is 'if the library name is X, then load Y', so the infinite recursion shouldn't be a problem for them.

I do think it makes sense that the overload that takes an Assembly would completely mimic the logic of if DllImport was used in the specified assembly, so I would like to have it go through all the extension points such that it:

  • invokes any registered per-assembly callback (DllImportResolver delegate)
  • calls LoadUnmanagedDll
  • fires ResolvingUnmanagedDll

But I did want to point out that then the API was added, it was an explicit decision to not do that.

@swaroop-sridhar do you have objections to changing this behaviour (if necessary, with the special casing for breaking infinite recursion)? Do you know of any examples that could have compat problems or if we gave out guidance when this was added that would lead to issues?

@swaroop-sridhar
Copy link
Contributor

I don't have an objection. When the API was added, the main motivating case was the implementation of DllMap. So, the requirements was to be able to call NativeLibrary.Load() from within DllImportResolver/LoadUnmanagedDll and load a (possibly different) DLL using NATIVE_DLL_SEARCH_DIRECTORIES.

So, the NativeLibrary.Load() primitive was considered a way to continue the execution, rather than restart it. However, a similar effect can be achieved using recursion breaking thread-static variables, as noted by Jan above. So, I'm OK with it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants