Implementation of ComponentDependencyResolver #8

vitek-karas · 2018-11-08T13:19:13Z

PInvokes into hostpolicy.dll (which should live next to the runtime and thus always be reachable).
If the PInvoke fails (missing hostpolicy.dll) we will fail for now.

Adds tests for the API into CoreCLR repo. The main reason is that with corerun
we can easily mock the hostpolicy.dll since there's none to start with.
Writing the same tests in CoreFX or any other place which starts the runtime through
hostpolicy would require test-only functionality to exist in either the class itself
or in the hostpolicy.

eerhardt · 2018-11-14T14:43:03Z

src/System.Private.CoreLib/src/System/Runtime/Loader/ComponentDependencyResolver.cs

+            string resource_search_paths);
+
+#pragma warning disable BCL0015 // Disable Pinvoke analyzer errors.
+        [DllImport("hostpolicy", CharSet = HostpolicyCharSet)]


Are we sure we want to take this dependency in CoreLib on hostpolicy? Also, it feels strange to have a corefx API depend on the hostpolicy assembly. What will happen in other scenarios that aren't using the dotnet.exe host? For example, Mono/Xamarin.

/cc @ericstj @jkotas

The actual behavior of the API is very runtime specific. On .NET Core it relies on .deps.json. On .NET Framework (if we would have it there) it would rely on something completely different (probably just directory layout). On Mono I assume it will also rely on different mechanism, whatever is it that Mono uses to describe dependencies.

The dependency on hostpolicy is for two reasons:

Right now to avoid code duplication - all the logic around .deps.json is in hostpolicy and duplicating it elsewhere is just asking for bugs.

Technological limitations - we don't have a JSON parser which could be part of CoreLib. The eventual long term plan is to use the new JSON parser from ASP.NET and move the code from hostpolicy to CoreLib for both startup .deps.json processing as well as for this functionality.

As for what happens if the runtime is not hosted through hostpolicy, currently this API would fail. I'm still thinking of what is doable for .NET Core 3 RTM, options are:

Fallback to directory based behavior (basically just like if there's no .deps.json)

Introduce a mechanism for custom coreclr hosts to provide the same service as hostpolicy. Currently they basically have to do that for startup, so it would make sense to have that.

Would it make more sense to add these APIs to the Microsoft.Extensions.DependencyModel library instead? This library already knows how to read and interpret .deps.json files.

The problem is layering:

That library has dependency on Newtonsoft.Json which is problematic (dependency on a specific version, which the app using the new API may not like, and so on)

Going forward we think we will need this API inside CoreLib itself. The main scenario is COM activation where CoreLib would use this API to be able to load the necessary dependencies for the assembly which contains the COM object being activated.

That library has dependency on Newtonsoft.Json which is problematic (dependency on a specific version, which the app using the new API may not like, and so on)

I was told that @steveharter was going to convert it to use the new JSON APIs instead on netcoreapp3.0+ (or netstandard2.1, if they make it into that). So that dependency will be going away in newer platforms. See https://github.com/dotnet/core-setup/issues/3311.

Going forward we think we will need this API inside CoreLib itself.

Ok, that is a good enough reason for me.

Thanks for the link to the issue - wasn't aware of it. Steve will not be the one doing it, that would be on me or the other Steve (@sdmaclea). I personally would very much prefer to move the code from hostpolicy which is currently all in C++ into corlib. That would solve several issues:

Our own productivity - C# is way faster to write/maintain than C++

No dependency on host (whatever that means)

Easy sharing of that functionality with other pieces - like this one

Ability to provide consistent diagnostics experience. Today one of the big problems of the host is that it has no ties to EventPipe/ETW, so diagnosing issues which span host and runtime is really tricky. (Adding support for ETW into host is a whole other topic, since it has its own PAL and everything...)

Another thing to consider, if you can't move the code down, is to have CoreCLR expose this as part of its hosting APIs and and have hostpolicy implement that hosting extension. That way you allow for any CoreCLR host to plug into this. dotnet/hostpolicy isn't the only coreclr host.

sdmaclea

I haven't looked at the tests in detail yet.

sdmaclea · 2018-11-14T18:14:50Z

src/System.Private.CoreLib/src/System/Runtime/Loader/ComponentDependencyResolver.cs

+            }
+            catch (DllNotFoundException dllNotFoundException)
+            {
+                throw new InvalidOperationException(SR.ComponentDependencyResolver_FailedToLoadHostpolicy, dllNotFoundException);


Seems like these two should throw a NotSupportedException

I guess it might make sense... it's true that these would be the "fallback" cases if we decide to implement some fallback behavior.

I guess I was thinking that the host was not supporting this....

sdmaclea · 2018-11-14T18:15:51Z

src/System.Private.CoreLib/src/System/Runtime/Loader/ComponentDependencyResolver.cs

+                // Something went wrong - report a failure
+                // TODO - once we have the ability to capture detailed error messages from the hostpolicy
+                // use it here to make the exception more actionable.
+                throw new InvalidOperationException(SR.Format(SR.ComponentDependencyResolver_FailedToResolveDependencies, componentAssemblyPath, returnCode));


This one might be the incorrect exception also.... It is not obvious why this would happen.

This is thrown when anything goes wrong with processing the .deps.json. With the future error redirection work, we will get exact error details, so the exception message will be much better. We could also change exception types based on the various error codes returned by the native code.

Typical example where this will fire:

The path to the component assembly is wrong (the file doesn't exist)

Invalid JSON in the .deps.json

Semantical error in the .deps.json - although there aren't that many cases where this would happen. The hostpolicy is pretty resilient to invalid inputs. (The one I know of is if there are duplicate entries in the .deps.json in which case hostpolicy doesn't know which one to pick)

sdmaclea · 2018-11-14T18:18:21Z

src/System.Private.CoreLib/src/System/Runtime/Loader/ComponentDependencyResolver.cs

+            //     (CoreCLR gets this and stores it as the culture name in the internal assembly name)
+            //     AssemblyName.CultureName is just a shortcut to AssemblyName.Culture.Name.
+            if (!string.IsNullOrEmpty(assemblyName.CultureName) && 
+                !string.Equals(assemblyName.CultureName, "neutral", StringComparison.OrdinalIgnoreCase))


I haven't seen the CultureName == "neutral" when looking at resolving event. Maybe there are code paths I haven't looked at. I wonder if this is dead native code.

sdmaclea · 2018-11-14T18:20:48Z

src/System.Private.CoreLib/src/System/Runtime/Loader/ComponentDependencyResolver.cs

+                // Search resource search paths by appending the culture name and the expected assembly file name.
+                // Copies the logic in BindSatelliteResourceByResourceRoots in CoreCLR.
+                // Note that the runtime will also probe APP_PATHS the same way, but that feature is effectively 
+                // being deprecated, so we chose to not support the same behavior for components.


This logic seems strange. I will look at this as I step through the code again...

swaroop-sridhar · 2018-11-15T21:18:57Z

src/System.Private.CoreLib/src/System/Runtime/Loader/ComponentDependencyResolver.cs

+            return null;
+        }
+
+        public string ResolveUnmanagedDllPath(string unmanagedDllName)


Please add a note here that the the CDR only resolves from the the component directory, deps.json paths, and simple name variants. For example: It doesn't respect DllSearchPath attributes on the Module or method. Further resolution should be handled by DllImport callbacks and resolvers.

Would it be fair to say something like:
"This only resolves the native library path by using the component directory, .deps.json information and simple name variants. It won't resolve libraries which would rely on the OS library resolution behavior. It should be used only as one part of a fully featured native library loading mechanism."
We should discuss the details here, as I need to understand what exactly the different options are...

PInvokes into hostpolicy.dll (which should live next to the runtime and thus always be reachable). If the PInvoke fails (missing hostpolicy.dll) we will fail for now. Adds tests for the API into CoreCLR repo. The main reason is that with corerun we can easily mock the hostpolicy.dll since there's none to start with. Writing the same tests in CoreFX or any other place which starts the runtime through hostpolicy would require test-only functionality to exist in either the class itself or in the hostpolicy.

Add native resolution tests

Registers error writer with the hostpolicy to receive detailed errors. Uses that in the exception. Modifications to the mock and the tests to be able to verify the functionality.

sdmaclea · 2019-04-10T22:58:38Z

@vitek-karas Seems you can close this PR

vitek-karas · 2019-04-11T07:23:53Z

Abandoning - this was used as the base change for the implementation of AssemblyDependencyResolver which is already in coreclr/corefx.

vitek-karas force-pushed the ComponentDependencyResolver branch from b94ac1b to 309de7b Compare November 13, 2018 13:38

eerhardt reviewed Nov 14, 2018

View reviewed changes

sdmaclea reviewed Nov 14, 2018

View reviewed changes

swaroop-sridhar reviewed Nov 15, 2018

View reviewed changes

vitek-karas added 4 commits December 5, 2018 11:33

Fix test project file to work outside of VS

30d1471

Better test cleanup to not leave artifacts on disk.

72ffcf4

CDR native resolution tests

7625911

Add native resolution tests

vitek-karas force-pushed the ComponentDependencyResolver branch from 309de7b to 7625911 Compare December 5, 2018 19:34

vitek-karas added 2 commits December 7, 2018 11:23

Implements detailed error reporting for ComponentDependencyResolver.

d7c0ef6

Registers error writer with the hostpolicy to receive detailed errors. Uses that in the exception. Modifications to the mock and the tests to be able to verify the functionality.

Revert overly eager cleanup

77c0bc9

jkoritzinsky mentioned this pull request Jan 8, 2019

Implement AssemblyDependencyResolver dotnet/coreclr#21896

Merged

vitek-karas closed this Apr 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of ComponentDependencyResolver #8

Implementation of ComponentDependencyResolver #8

vitek-karas commented Nov 8, 2018

eerhardt Nov 14, 2018

vitek-karas Nov 14, 2018

eerhardt Nov 14, 2018

vitek-karas Nov 14, 2018

eerhardt Nov 14, 2018

vitek-karas Nov 14, 2018

ericstj Nov 14, 2018

sdmaclea left a comment

sdmaclea Nov 14, 2018

vitek-karas Nov 14, 2018

sdmaclea Nov 14, 2018

sdmaclea Nov 14, 2018

vitek-karas Nov 14, 2018

sdmaclea Nov 14, 2018

sdmaclea Nov 14, 2018

swaroop-sridhar Nov 15, 2018

vitek-karas Nov 16, 2018

sdmaclea commented Apr 10, 2019

vitek-karas commented Apr 11, 2019

Implementation of ComponentDependencyResolver #8

Implementation of ComponentDependencyResolver #8

Conversation

vitek-karas commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdmaclea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdmaclea commented Apr 10, 2019

vitek-karas commented Apr 11, 2019