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

WIP: Design proposal for dynamic component loading. #47

Closed

Conversation

vitek-karas
Copy link
Member

This design proposal is trying to address the lack of easy-to-use functionality to dynamically load components with their own dependencies.

@vitek-karas
Copy link
Member Author

Marked as WIP as parts of this proposal are still being worked on.

Copy link
Contributor

@sdmaclea sdmaclea left a comment

Choose a reason for hiding this comment

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

Overall looks a good approach. I think we should start prototyping.

* The new load context is initialized by specifying the full path to the main assembly of the component to load.
* It will look for the `.deps.json` next to that assembly to determine its dependencies. Lack of `.deps.json` will be treated in the same way it is today for executable apps - that is all the assemblies in the same folder will be used as dependencies.
* Parsing and understanding of the `.deps.json` will be performed by the same host components which do this for executable apps (so same behavior/quirks/bugs, very little code duplication). Specifically `hostpolicy.dll` is the component which parses `.deps.json` during app startup. See [host-components](https://github.com/dotnet/core-setup/blob/master/Documentation/design-docs/host-components.md) for more details. If this functionality is required when running with a custom host, the said host would need to provide this functionality to the runtime.
* If the component has `.runtimeconfig.json` and/or `.runtimeconfig.dev.json` it will only be used to verify runtime version and provide probing paths.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given @natemcmaster 's post here
https://natemcmaster.com/blog/2017/12/21/netcore-primitives/ and the related cited documentation in CLI...

It seems the .runtimeconfig.json is intended for the human edited configuration. While this is in its infancy, I wonder if it is the only config file we should support. Once we work through exactly what we want we can adjust the tooling to write the .deps.json file

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry - probably missing the point of your question.
.runtimeconfig.json serves different purpose than .deps.json.
The former is used to locate the runtime and the runtime options to start the runtime with. It also stores framework dependencies - this is somewhat weird since the "dependencies" part of that should be in .deps.json logically. On the other hand, the way to find the runtime is to locate the lowest-level framework, so that part needs to be in .runtimeconfig.json.
In a way the runtime-config part is meant to be human editted. The framework dependencies are generally machine generated as those fall out of the compilation process.
The latter is used to locate specific assembly dependencies. This almost entirely a result of the compilation process and thus machine generated.

Please note that vast majority of dynamically loaded components will not have .runtimeconfig.json. The proposed new functionality is about finding assembly dependencies and as such it's not really tied to the .runtimeconfig.json (other than the probing paths), on the other hand it is very much tied to .deps.json

* Auto-upgrade - components will auto-upgrade to the version of a dependency they share with the app (downgrade will not occur, in that case the dependency would be loaded in isolation). This can lead to incompatibilities.

Open questions:
* Which is the default behavior?
Copy link
Contributor

Choose a reason for hiding this comment

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

My initial preference was for the Full Isolation case, but the more I think about it the more I prefer the Prefer default implementation.

I would rename it Prefer parent and use the hierarchy. This would help allow for plugin families. It could easily be mimic with natural hierarchical directory structure.

Choose a reason for hiding this comment

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

Like class loaders in Java? Perhaps than assemblies loading can have an API as well so it may decide whether to give a chance to parent to load assembly first. The same way as class loaders works.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SergeyKanzhelev I have only very limited knowledge of Java in general. So far what I've read about class loaders they are very similar to .NET Core's AssemblyLoadContext. This class already provides ways for a custom implementation to defer to the parent first if the custom code wants to do that.

This proposal heavily relies on AssemblyLoadContext as the way to achieve isolation. I guess one way of thinking about this proposal is that we're trying to add an easier-to-use class loader to dynamically load components with their dependencies. But again, I know very little about the Java world, so maybe I'm completely wrong.

Choose a reason for hiding this comment

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

I need to read about AssemblyLoadContext more.

Yes, I was mostly referring to the fact that class loaders can be custom coded to decide on loading ordering themselves. And also there are few well-known class loaders which user is aware of and can decide to load some assemblies there so they will share statics.

If simplified version if enough for most scenarios - it should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a description of AssemblyLoadContext. It's not entirely up-to-date, but the basics are there. Also added this link to the doc. Let me know if there are unclear parts, I'll go fix the other doc as well.

Choose a reason for hiding this comment

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

I know my comment won't be of any help in terms of how you guys would proceed but I just wanted to say, with 20 or so many years of software development background, the AssemblyLoadContext is very hard to understand. Either due to the API design or due to the lack of documentation (samples rather than API description), I don't know. Just wanted to share how I feel about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@CoskunSunali Thanks. We are hearing the same thing from lots of customers.

* Which is the default behavior?
* Does the framework implement more than one behavior and lets users choose?
* In the "prefer default" behavior - what does it mean for the parent context to "satisfy" the dependency? Does it mean exact version match, or does it allow auto-upgrade (patch, minor or even major)? Do we even allow downgrades?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the default should be auto upgrade.

However, it seems this should also be configurable. I would suggest a new config property in the .runtime.config

Copy link
Member Author

Choose a reason for hiding this comment

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

.runtimeconfig.json already has settings which control auto-upgrade - also called roll-forward (I should use that term instead). See this doc.

Those apply to frameworks though, not assemblies. But in the context of components (since those rely on framework fromt he app) it would make sense to use them for individual assemblies as well.


"Advanced" version which would return an ALC instance and not just the main assembly. Allows for additional changes to the `AssemblyLoadContext`, like registering an event handler to the `Resolving` event for example.
Also adds the ability to specify:
* `fallbackContext` which is the `AssemblyLoadContext` to defer to when the assembly resolution is not possible in this current context. By default this is the `AssemblyLoadContext.Default` (the app's context). This allows for creating effectively parent-child relationships between the load contexts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the parent-child relationship and the various possible strategies mentioned below, it seems this would be better named parentContext


Open questions:
* Which is the default behavior?
* Does the framework implement more than one behavior and lets users choose?
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkotas suggested in another thread that we defer the convenience API until we get a feel for which is the right approach.

When we were prototyping hardware intrinsics for 2.1, late in the cycle we pushed the work into experimental nuget packages. I am wondering whether that might be the right approach here.

I don't see a reason why we couldn't create a nuget package to expose these API's experimentally so that we can get real world experience and hand on feedback.

The experimental package(s) could be used to provide sample code for the usage of the existing 2.1 AssemblyLoadContext and help us develop proper documentation.

Copy link
Member

Choose a reason for hiding this comment

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

To help move forward providing an instance method on AssemblyLoadConext to enable the LoadWithDependencies seems like a reasonable way to acheive the balance we have been discussing. It will lead people towards going for isolation, but enables them to choose which ALC to load into. We can then choose which version is most appropriate for Assembly.Load*WithDependencies (eg. the default)

* There's no pollution of the parent context - no new files will be loaded into the parent context.
* Auto-upgrade - components will auto-upgrade to the version of a dependency they share with the app (downgrade will not occur, in that case the dependency would be loaded in isolation). This can lead to incompatibilities.

Open questions:

Choose a reason for hiding this comment

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

Important consideration for me is versioning of EventSource names and manifests when multiple versions are loaded into the process. Same for diagnostics source.

It is my understanding that all isolation models will allow to subscribe to event and diagnostics sources from the loaded assembly as those sources are defined in system assemblies. If so - some naming issue may occur.

CC: @vancem

Copy link
Member Author

Choose a reason for hiding this comment

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

Please note that isolation is not a new feature. It has been available for a long time now. It's done through the AssemblyLoadContext class which is not new. Also some methods on Assembly provide isolation (Assembly.LoadFile loads the specified file in isolation). So it's already possible to load even the same assembly twice (or two similar assemblies and so on). I don't know if there's been any thinking on the impact of this capability on the EventSource mechanisms. I'll let @vancem or @brianrob to comment on that.

Choose a reason for hiding this comment

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

@vancem, @brianrob what do you think? Can you confirm that loading in separate AssemblyLoadContext will still make everybody use the "global" Event and Diagnostics Source?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm that from assembly binding perspective the System.Private.CoreLib which is where most of the code for EventSource and alike lives is only ever loaded into the app once. In fact corelib is special and the runtime will not allow loading a second copy even if custom code would specifically ask for it. But that doesn't mean that the event source code doesn't have some internal data structures which might have issues with multiple copies of the same assembly.

Copy link

Choose a reason for hiding this comment

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

@SergeyKanzhelev - I view all this loader stuff as pretty orthogonal to event/source naming issues in EventSource/DiagnosticSource. (Yes there are issues but this feature does not make the problem any better or worse).

The one place I can think of that is a issue is that TODAY we make it illegal to create two EventSources with the same name in the same AppDomain. We did this for relatively weak reasons (it is a perf issue). This behavior WILL cause grief if an assembly is loaded twice (because of isolation). It will call the SHARED System.Private.Corelib version of EventSource which will fail because the name is already used. I have always been ambivalent about this rule, so I don't mind if we stop checking for it.

As far as the rest goes, I think it is roughly orthogonal. The names WILL overlap but that is good (you think of them as the 'same' events really). For DiagnosticSource, there will be a separate publication point for each System.Diagnostic.DiagnosticSource assembly that is loaded (since there is a static in their that you subscribe to to get events.

So we may wish to relax the uniqueness check in EventSource, but otherwise I think it will be OK. If you have specific concerns, we can walk through specific cases in more detail.

Choose a reason for hiding this comment

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

This behavior WILL cause grief if an assembly is loaded twice (because of isolation). It will call the SHARED System.Private.Corelib version of EventSource which will fail because the name is already used.

So custom EventSource creation will crash? Potentially an entire process? Or this will be handled? In any case - this may become a common case with plugins using libraries like Application Insights for telemetry reporting. Application Insights SDK instantiate an EventSource for troubleshooting.

For DiagnosticSource, there will be a separate publication point for each System.Diagnostic.DiagnosticSource assembly that is loaded

Will different DiagnosticSource-s share the AsyncLocal (Activity.Current)? If not - distributed tracing scenarios like Activity started in ASP.NET core host and than child activity created inside the plugin would not work. Is there anything that can be done to special-case the DiagnosticsSource?

* MSBuild tasks - all tasks in MSBuild are dynamically loaded. Some tasks come with additional dependencies which can collide with each other or MSBuild itself as well.
* Roslyn analyzers - similar to MSBuild tasks, the Roslyn compiler dynamically loads analyzers which are separate components with potentially conflicting dependencies.
* XUnit loading tests - the test runner acts as an app and the test is loaded dynamically. The test can have any number of dependencies. Finding and resolving those dependencies is challenging.
* ASP .NET's `dotnet watch` - ability to dynamically reload an app without restarting the process. Each version of the app is inherently in collision with any previous version. The old version should be unloaded.

Choose a reason for hiding this comment

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

Please include ASP.NET Core plugins like z-pages (see performance-profiling-controller.md) and monitoring tools like Application Insights.

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@SergeyKanzhelev In which way do you think the performance profiling controller is related to the dynamic component loading? I can potentially see some use cases, but I don't see that as a good example of the scenarios we want to support (not saying it's not applicable).

Monitoring tools and other profiler-like capabilities are probably more related to the [statup-hook] (https://github.com/dotnet/core-setup/blob/master/Documentation/design-docs/host-startup-hook.md) functionality. Also this new feature will explicitely not customize startup loading in any way. It will be on-demand - that is called by a user code. It is possible for startup-hooks to use this feature to load their own dependencies though.

I guess I'm missing the point here though - can you please describe in a bit more detail how do you think these are related?

Choose a reason for hiding this comment

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

The discussion was how one can load ASP.NET components to implement profiling controller into an application that has no ASP.NET stack loaded or has it's own version of ASP.NET stack. Also those profiling controllers may implement some pluggable model to load data collection plug-ins on demand in runtime. Versioning and isolation was one of the discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

For isolation, AssemblyLoadContext is definitely the solution. For the on demand runtime loading - that would be this feature (it's possible, but rather hard right now). Thanks for pointing that out - I'll include it in the list.

* MSBuild tasks - all tasks in MSBuild are dynamically loaded. Some tasks come with additional dependencies which can collide with each other or MSBuild itself as well.
* Roslyn analyzers - similar to MSBuild tasks, the Roslyn compiler dynamically loads analyzers which are separate components with potentially conflicting dependencies.
* XUnit loading tests - the test runner acts as an app and the test is loaded dynamically. The test can have any number of dependencies. Finding and resolving those dependencies is challenging.
* ASP .NET's `dotnet watch` - ability to dynamically reload an app without restarting the process. Each version of the app is inherently in collision with any previous version. The old version should be unloaded.

Choose a reason for hiding this comment

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

another scenario is startup hook. Perhaps spec of profiler hook should be updated to suggest using isolation loading for those hooks. @sbomer

Copy link
Member

Choose a reason for hiding this comment

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

Are you referring to ICorProfiler when you say profiler? That is native and is not impacted by this work. The startup hook is interesting though.

Choose a reason for hiding this comment

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

no, I misspoke. I meant startup hook I referenced.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Sorry about the previous comment, didn't see this one yet). I agree that using isolation for startup hooks makes sense in some cases. But that functionality is available already. This feature is actually not about providing isolation per-se. It's about the ability to load components with dependencies (which has been hard so far). As seen in the discussion in this proposal, it's likely we will provide a way to do this loading both in isolation or not.

@natemcmaster
Copy link
Contributor

I've implemented a lot of this already, and been down the road of quirks you'll have to handle if you want a managed AssemblyLoadContext to imitate corehost's deps.json parsing. Feel free to use this as a prototype. https://github.com/natemcmaster/DotNetCorePlugins

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Overall I like the approach. I still feel we need some thinking around if ALC.LoadWIthDependencies is a factory method or an instance.

```csharp
class AssemblyLoadContext
{
public static AssemblyLoadContext CreateForAssemblyWithDependencies(
Copy link
Member

Choose a reason for hiding this comment

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

I feel we are going to have scenarios where we will want multiple plug-ins loaded into the same assembly load context. I prefer this factory method to be an instance method so that developers can choose.


Open questions:
* Which is the default behavior?
* Does the framework implement more than one behavior and lets users choose?
Copy link
Member

Choose a reason for hiding this comment

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

To help move forward providing an instance method on AssemblyLoadConext to enable the LoadWithDependencies seems like a reasonable way to acheive the balance we have been discussing. It will lead people towards going for isolation, but enables them to choose which ALC to load into. We can then choose which version is most appropriate for Assembly.Load*WithDependencies (eg. the default)

@vitek-karas
Copy link
Member Author

@natemcmaster I'm very familiar with your code - thanks a lot for putting that together. Please note that we're explicitly proposing to not implement the .deps.json loading and resolution in managed code, exactly to avoid duplication and possibly a new set of quircks/bugs. The proposal is to call down to the hosting code (hostpolicy.dll) and use the exact same code to do this as is already used during startup.

@natemcmaster
Copy link
Contributor

The proposal is to call down to the hosting code (hostpolicy.dll) and use the exact same code to do this as is already used during startup.

+💯. Awesome, that's a much better way to do this.

@vancem
Copy link

vancem commented Oct 1, 2018

If I understand this feature properly it does not add anything truly new to the runtime (users COULD implement it themselves with AssemblyLoadContext), but is effectively a 'shortcut' for a common case (a plugin with non-trivial dependencies. If this is not true, we should add the (hopefully small) additions to AssemblyLoadContext to expose the primitives to make it true.

My bigest piece of advice is that in the past the loader has become a mess of special cases that became very hard to use or reason able. We should try to avoid that here. In particular.

  • It is OK to have policy (arbitrary decisions about how things are looked up), but ideally there are 'fallbacks' (more primitive APIs) that allow users to 'take control' if our policy was bad.

  • Ideally our 'documenation' of that policy is some source code that is so simple and easy to understand, it serves as both code and documentation.

  • From what I can tell the big 'value' of this feature is simply that we have code for dealing with deps files and probing location that people want to reuse (and stay consistant with what the runtime does). Thus ideally

  1. We have some managed APIs that create a 'object model' for this. (e.g. some APIs for getting at Deps information).
  2. Write the 'policy' in terms of this
  3. Iterate on that object model API until the policy code is 'clean' and close to minimal (thus it becomes easy for others to have their own policy as needed).
  4. Ideally we keep thinking about making it SIMPLER while preserving the necessary functionality/flexibility.

This is important because most of the open issues have to do with having different policies or extended features etc. We want these to be easy to add or simply have users do it (because it is that easy).

Note that we can keep this object model 'private' for a while if we want (until we are comfortable that we won't want to change it).

The basic point is that loaders really don't DO much (they take names and find the assembly that 'matches'), but they are SHARED (that is the whole app uses it), which is what forces it to have 1000 masters (each needing a tweek).

@Levridge-Tory
Copy link

What is the status of this proposal? It is now over a year old and with 3.0 (I haven't tried 3.1 yet) I am still unable to dynamically load an assembly with AssemblyLoadContext and have the dependent assemblies (in deps.json) automatically loaded.I see this is still open. Is there a plan to included this? Am I missing something?

@jeffschwMSFT
Copy link
Member

Largely this design was implemented in .NET Core 3. Here are a few links that hopefully will help:
samples: https://github.com/dotnet/samples/tree/master/core/extensions/AppWithPlugin
AssemblyDependencyResolver: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblydependencyresolver?view=netcore-3.1

@terrajobst
Copy link
Contributor

@vitek-karas what's the status of this?

@vitek-karas
Copy link
Member Author

A simpler approach was taken for .NET Core 3.0

  • Incremental improvements to AssemblyLoadContext - mostly to make it easier to implement a custom one
  • Ability to unload AssemblyLoadContext by marking it as collectible
  • Introduced AssemblyDependencyResolver which makes it simple to implement a typical load context for plugins
  • Samples and guides for working with custom load contexts.

The higher-level API to make it even easier to load plugins without the need for an explicit custom load context is not part of .NET Core 3.0. There are also no immediate plans to add that functionality to .NET. As such I'm closing this proposal.

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.

10 participants