Document how unmanaged libraries are loaded with AssemblyLoadContext

[natemcmaster]

On Windows, System.Data.SqlClient currently p/invokes to a native component sni.dll via DllImport [DllImport("sni.dll")]. This is inconsistent with other Dllimport calls in corefx that do not include the ".dll" extension in the native dll name. This causes inconsistent behavior in other parts of the stack.

e.g. AssemblyLoadContext.LoadUnmanagedDll(string unmanagedDllName) is called with unmanagedDllName == "sni.dll", but for other libraries the extension is left off. This means AssemblyLoadContext is usually implemented to look for "sni", not "sni.dll".

cref https://github.com/dotnet/cli/issues/2289#issuecomment-208655706

Update
Decided the behavior is fine, but we should document it as it's non-obvious.

[saurabh500]

@natemcmaster

This is inconsistent with other Dllimport calls in corefx that do not include the ".dll" extension in the native dll name.

Based on the Interop Libraries that are being used in Corefx, I see that the Library names have the .dll appended to them.

https://github.com/dotnet/corefx/blob/master/src/Common/src/Interop/Windows/Interop.Libraries.cs

Is this bug specific to a sni.dll? Why do the other dll imports with the extension appended not cause any issue?

cc @schellap @stephentoub

[natemcmaster]

I see that the Library names have the .dll appended to them.

Then I guess this discussion should be broader. It was my understanding that coreclr took care of appending .dll/.so/.dylib based on the platform. dotnet/coreclr#1248. Accordingly, managed code shouldn't need ".dll" explicitly in the unmanaged library name.

Since this appears to be broader than just SqlClient, this issue could probably be merged with the disucssion of this behavior in other places, such as https://github.com/dotnet/coreclr/issues/930 or https://github.com/dotnet/coreclr/issues/444.

Or perhaps this can be considered a bug on the AssemblyLoadContext API.

[saurabh500]

this appears to be broader than just SqlClient,

@joshfree can you direct this issue accordingly?

[stephentoub]

Accordingly, managed code shouldn't need ".dll" explicitly in the unmanaged library name.

It doesn't need it, but there's nothing wrong with providing it. I don't see a problem here.

Or perhaps this can be considered a bug on the AssemblyLoadContext API.

What's the buggy behavior? Isn't it just passing through the name of the library that was specified?
cc: @gkhanna79

[natemcmaster]

The behavior that seems odd is that the parameter unmanagedDllName in AssemblyLoadContext.LoadUnmanagedDll(string unmanagedDllName) can receive inconsistent arguments. In some cases (as with corefx libaries), this name may include ".dll". Implementers of AssemblyLoadContext are left to manage this possibility, essentially repeating the work done by coreclr to normalize dll names.

[gkhanna79]

Looking at NDirect::LoadLibraryModule, the call made to AssemblyLoadContext.LoadUnManagedDll happens before CoreCLR interop layer attempts extension name processing.

@adityamandaleeka Do you want to look into how to account for extension normalization before AssemblyLoadContext is invoked?

[adityamandaleeka]

@natemcmaster @stephentoub

The prefix/suffix logic we do in the DLL import code only adds the prefix and/or suffix to the name provided. If you PInvoke with [DllImport("foo.dll")] when 'foo' is actually a .dylib or a .so, we will fail to load it. However, if 'foo' is really a .dll, it will be loaded correctly. Is this consistent with the behavior you're seeing?

[natemcmaster]

The prefix/suffix logic seems to work fine when relying on the default load behavior. But I'm trying to implement the abstract class AssemblyLoadContext. The question that isn't clear from the API docs is this:

DllImport("foo.dll")
          -- coreclr calls -->  
                        AssemblyLoadContext.LoadUnmanagedDll(x)

Which is true?
x == "foo.dll" or x == "foo"

Right now, it depends on what was in DllImport, which can vary.

I would have expected:

System.Runtime.Loader.AssemblyLoadContext.LoadUnmanagedDll(string unmanagedDllName)

unmanagedDllName never includes .dll, .dylib, .so etc

System.Runtime.Loader.AssemblyLoadContext.LoadUnmanagedDllFromPath(string unmanagedDllPath)

unmanagedDllPath always contains .dll, .dylib, .so etc

[adityamandaleeka]

I understand. Right now I believe that the string is passed through the system essentially untouched. Your suggestion (LoadUnmanagedDll doesn't include the suffix in the names) makes sense, but it means that we have to manipulate what the user passed in, which strikes me as potentially unsafe.

Consider the case where the user has named their native binary with a non-standard extension, such as using 'foo.dll' on a Mac. If we cut off the extension, it will be up to the host to somehow figure out what to do in that case right?

[natemcmaster]

After considering this, I think perhaps the better solution is leave the implementation as is, but just fix up API docs to make it clear that p/invoke essential flows through directly to the assemblyloadcontext call. The less magic the better.

[adityamandaleeka]

@natemcmaster Agreed. 👍

[AlexGhiondea]

It looks like we need to improve our documentation around this.

@karelz how should we track documentation issues?

[karelz]

'documentation' label is the best at this moment for API docs.
We will flush out the whole API docs story in December/January. Until then, let's just mark the bugs with 'documentation' label.

[rainersigwald]

Is this superseded now by AssemblyDependencyResolver.ResolveUnmanagedDllToPath?

[dotnet-policy-service]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

[dotnet-policy-service]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.