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

Support Usage of Library in Non-Default AssemblyLoadContext(s) #111

Merged
merged 5 commits into from Dec 29, 2019

Conversation

Sewer56
Copy link
Contributor

@Sewer56 Sewer56 commented Dec 17, 2019

For users of this library in more advanced scenarios, this pull request adds the ability to use the library outside of the Default AssemblyLoadContext. The Readme was also updated to make note of this use case.

More details behind the motives of this PR can be found here: #110

Tests

Manually checked against all existing runtimes and configurations and an actual real program that requires this change.

I didn't necessarily add any additional tests, since as far as I'm concerned, no actual real logic was changed. At the core, all this PR does is replace two references to AssemblyLoadContext.Default with a user defined ALC, which defaults to Default. A method was added to ALC Builder and constructor of ManagedLoadContext (internal, no breaking public API changes) changed.

Only real thing of note is ManagedLoadContext.Load() now doesn't return null on success, and instead the assembly directly, since we're not asking the framework anymore to default to the default ALC. This is a non-breaking change.

Copy link
Owner

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this PR, @Sewer56! I think I'll be okay adding this feature, but I'd like to hear your answers to a few comments below.

Also, I trust you when you save you've done manual tests, but tests are still important to avoid regressions in the future, so let's find a way to write some first.

// Older versions used to return null here such that returned assembly would be resolved from the default ALC.
// However, with the addition of custom default ALCs, the Default ALC may not be the user's chosen ALC when
// this context was built. As such, we simply return the Assembly from the user's chosen default load context.
return defaultAssembly;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one change that I am concerned about. Which context will this instance of Assembly belong to? The reason for returning null was to ensure the default context is used, and I don't know what behavior we will get here. I'd like to see some automated tests added that validate this is actually working before we move forward with merging this.

Copy link
Contributor Author

@Sewer56 Sewer56 Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually the exact line that makes this whole PR work.
In Core, assemblies and contexts have a 1:1 mapping to my understanding. This comes from recently added ALC documentation on MSDN and some manual debugging to confirm.

In other words, when loaded by an ALC, the Assembly instance belongs to that ALC, just like plugin interfaces. Having the object cross ALCs doesn't mean that the object will be loaded into the foreign ALC.

You should be able to verify this in a debugger by creating an ALC "B" and using LoadFromAssemblyName, manually with that ALC. (Note: Create the AssemblyName manually without loading the assembly). Then, once that Assembly is loaded you can verify the assemblies loaded into a context using the Assemblies property of an ALC. The assembly will not be in the current context, only in ALC "B"'s Assemblies list.

Also random note: I noticed this after I made the PR but this MSDN page exists: Creating App with Plugin Support , it does pretty much the same thing as this PR in terms of returning the Assembly instance.

src/Plugins/PluginConfig.cs Outdated Show resolved Hide resolved
src/Plugins/PublicAPI.Shipped.txt Show resolved Hide resolved
@Sewer56
Copy link
Contributor Author

Sewer56 commented Dec 24, 2019

Re: Adding a test.

I think an appropriate basic test would be creating an ALC that uses DotNetCorePlugins to host its own plugin system.

I am currently running this PR 'in production' in Reloaded-II which you've already seen at some point before. Reloaded II's setup is that it's loaded using Native Runtime Hosting (via HostFXR), forcing it to be loaded outside of the default load context. Previously I used a very terrible hack involving Costura.Fody, that isn't really worth explaining. This is niche software though and I would reckon only 50-100 unique people have ran this since it's been added.

I think I should be able to add such a test no problem. I'll start working on it probably tomorrow and have something cooked rather quickly. For now I'm going to kick back and spend some time (than usual) with family and friends for today.

@Sewer56
Copy link
Contributor Author

Sewer56 commented Dec 25, 2019

Alright @natemcmaster , I changed the 'Default' load context in the configuration to the context of the currently executing assembly as agreed upon and added a test for the main use case behind the PR.

The test was a bit nontrivial to add but is quite simple. It is composed of a "Plugins inside Plugins" scenario, whereby we create a plugin (WithOwnPlugins,that loads in non-default ALC), that in turn, loads its own plugins.

To make the test as 'Pure' as possible, I was extra careful crafting it. I made absolutely sure that any unification of types with the Default (test runner's) ALC is impossible.

  • The test runner ALC does not load the plugin or any of the plugin's dependencies.
  • The test ensures ALC of the plugin and the test runner are not the same,
  • The plugin checks against the test runner ALC whether it has the plugin's exported plugin types loaded. (Should never be true unless a breaking change elsewhere in the library happens)

Hardest part of implementing this PR was actually figuring out how to build this thing. Particularly the following points:

  • Avoiding the loading of assemblies (as above).
  • Multi-targeting. (Plugin references McMaster.NETCore.Plugins)
  • Locating a separately compiled plugin from inside a plugin. (Locating plugins of WithOwnPlugins)

I settled for adding another item definition MultitargetTestProject, which is a copy of TestProject that doesn't undefine the TargetFrameworks property. The plugins of WithOwnPlugins meanwhile get copied to the output directory of WithOwnPlugins, using project references but without a direct dependency reference (ReferenceOutputAssembly="false"). I made them copy to a subdirectory, Plugins, the reason why is explained inside the test.

Should likely be good for merging, see what you think.

Copy link
Owner

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really excellent work. Thanks @Sewer56 for the updates and careful work you've done to add tests. I'm going to merge this and this will be part of the 1.0.0 release that I'm planning on pushing out in a few days.

Also, I'll make sure to add you to the release notes :)

@natemcmaster natemcmaster merged commit a634d3b into natemcmaster:master Dec 29, 2019
@Sewer56 Sewer56 deleted the master branch October 11, 2020 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants