Fix NativeDllHandler to not throw when file is not found

[daxian-dbw]

PR Summary

Fix NativeDllHandler to not throw when file is not found.

Today, the NativeDllHandler calls NativeLibrary.Load(fullName) even if the file doesn't exist. This is not right, it should return IntPtr.Zero to let the runtime try the next resolution approach, if there is one.
Also, this behavior results in the exception message to be Unable to load DLL 'F:\win-x64\nativedll.dll' or one of its dependencies: The specified module could not be found, which is confusing because there was never an explicit loading of a dll with that path.
The real exception generated from runtime should be Unable to load DLL 'nativedll' or one of its dependencies: The specified module could not be found.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

[ghost]

Hello @TravisEz13!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 24 hours, a condition that will be fulfilled in about 23 hours 19 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[iSazonov]

Today, the NativeDllHandler calls NativeLibrary.Load(fullName) even if the file doesn't exist. This is not right, it should return IntPtr.Zero to let the runtime try the next resolution approach, if there is one.

https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.nativelibrary.load?view=netcore-3.1

Docs says NativeLibrary.Load() throws DllNotFoundException if the file does not exist.
So the exception is a root cause of the issue.
I suggest to use NativeLibrary.TryLoad() method to avoid explicit file existence check. This also addresses BadImageFormatException if a file.dll exists but it is not valid dll.

NativeLibrary.Load Method (System.Runtime.InteropServices)

Provides a simple API for loading a native library that wraps the OS loader and uses default flags.

[daxian-dbw]

avoid explicit file existence check.

@iSazonov How would that add any benefit? There will be a file check in NativeLibrary.TryLoad() anyways. It makes more sense to check here and avoid an unnecessary method call (p/invoke).

[TravisEz13]

@msftclas make sure @PaulHigin has a chance to review before you merge

[iSazonov]

How would that add any benefit? There will be a file check in NativeLibrary.TryLoad() anyways. It makes more sense to check here and avoid an unnecessary method call (p/invoke).

The resolver is last resort resolver. In most scenarios all native dll-s are loaded before the resolver. The resolver is called only for one scenario - last attempt to load arch-dependent native dll. Normally the attempt is successful and we do not need to check for the dll file before p/invoke. Any fail in native dll loading is rare and it is a bug (we should again not worry about dll file check) - either dll is absent or corrupted - both these cases TryLoad() can catch.

[daxian-dbw]

The resolver is last resort resolver.

This is just the current runtime behavior. It's possible another resolving approach could be added after the ResolvingUnmanagedDll event in future. And that won't be a breaking change unless the handler registered to the event is not returning IntPtr.Zero when it fails to resolve.

This cause problem to application that hosts powershell, e.g. dotnet-interactive. Both the application code and powershell is using the default load context, so this event handler will kick in for all native library loading, even if it's triggered by the application code instead of a powershell module. With the current code, the exception thrown from this handler is super confusing, also the stack trace makes PowerShell look like a culprit to blame.

both these cases TryLoad() can catch.

Using TryLoad here is wrong because it will never throw an exception even if the DLL exists but somehow cannot be loaded.
When the file actually exists, an exception should be thrown when it cannot be loaded.

[iSazonov]

so this event handler will kick in for all native library loading, even if it's triggered by the application code instead of a powershell module.

In the case we should catch and log any exception and continue resolving process.

[daxian-dbw]

Actually, on a second thought, I think you are right about using TryLoad.
If we cannot load the DLL (loading fails), it also indicates the resolution failed, isn't it?
In that case, it's better to let the runtime to throw the general exception Unable to load DLL 'nativedll' instead of PowerShell throwing out an exception with a specific path, because that path might not mean for a native DLL at all.

I will update the code. Thanks @iSazonov!

[ghost]

🎉v7.0.0-rc.3 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes