Ability to isolate script execution with AssemblyLoadContext #631
Conversation
…ppDomain and AssemblyLoadContext scenarios.
|
Just out of curiosity. Where you plan to plug this into Roslyn scripting? Take a look here for some of the details around this If I remember correctly this is where it stopped for me since everything in this area is either sealed or internal :) |
The current implementation of Roslyn already covers this by using its own |
|
The second set of changes presented above allows to configure Since we want to use The API of I've also used nullable notation for that addition as the folks who switched to nullable checks reap the immediate benefits by seeing a question mark hints in IntelliSense dropdown, meaning that corresponding value is optional. When (To be continued) |
Yes, I know about that one :) I am the one who wrote most of the assembly loading stuff :) What I meant was how to plug the new custom |
We will plug using Specific sample: https://github.com/dotnet/coreclr/blob/master/Documentation/design-docs/AssemblyLoadContext.ContextualReflection.md#maintaining-and-restoring-original-behavior. Note that all |
Ah, I did not know about that one😊 Never a day without learning something new 😊 With the regards to the dependency on |
…extual reflection for .NET Core 3.0+ hosts.
|
The third set of changes presented above completes the local support of As we know, the script is compiled to a temp folder and its dependencies are laying around at the same folder. The default logic embodied into In order to achieve that, the Other than providing a functional assembly loading, we should propagate our (To be continued) |
|
The last set of changes presented above glues and propagates a custom Here is an example of how it can be used: The sample demonstrates running the script in an isolated assembly load context. When script is being run in this way, its assemblies do not clash with the host assemblies. If we do not specify a custom The commits above finish the core implementation of the feature. |
This module is used to the fullest extent here, so we would need almost all of the bits :) I would vote against inlining of that particular one. If you dive deeper into the details, there are quite a few intricate aspects that should be handled in a particular and not always straightforward way. This module encompasses maybe nearly 15 years of our work with .NET and custom assembly loading, but still, there are the days when we say "Aha!", learn something new, and stream those improvements into I would really prefer if the maximum amount of living beings would benefit from that expertise, so really want to leave it upstreamed, not inlined. Reinventing the wheel is not energy-efficient and at some point of life you just want to rely on quality (which is hard to find, BTW). However, a final decision is always yours and I would follow it. |
We might be able to live with that although it is not my decision alone, @filipw also need to give his stamp of approval 👍 Just a couple of other things to be aware of
This currently spits out |
|
The changes so far only addressed the API side of the feature, so The next commits will gradually address those scenarios. |
|
With the commit above, isolation is turned on for script file execution via It has an interesting behavior. The script that references a newer Newtonsoft.Json is isolated correctly: It produces the expected output Without the isolation in place, the script just errors out with But, the script referencing an older Newtonsoft.Json version Should we really aim for the full assembly isolation? The existing behavior assuming assembly backward compatibility and partial isolation looks pretty sane. |
|
IMHO I think for this to have real value, we should try to reach for the stars and provide 100% isolation. Otherwise we are possibly left with the same or additional edge cases related to inference from the host. |
Agree. Meanwhile I was able to achieve 100% isolation in a prototype implementation. Should I continue in this pull request or is it better to split in two? The first one - API support which is safe and already covers some use cases, the second pull request - full isolation, including |
|
I think we should try to keep this in a single PR. The changes so far is not too comprehensive. It would also be easier to review these changes as a whole. Great work btw. Really appreciate your efforts on this 👍👌 |
|
The current implementation reaches the full assembly isolation at API and There is a remaining thing: the full isolation of interactive runner. Currently it runs with a partial isolation which is pretty good. I've tried to make it fully isolated and was blocked by this line in Roslyn: https://github.com/dotnet/roslyn/blob/2f6768efa3f5575bae411524bb11364b2208c2c4/src/Scripting/Core/Hosting/AssemblyLoader/CoreAssemblyLoaderImpl.cs#L35 Unfortunately an explicit The pull requested is finished and ready for review/merge. |
|
Impressive work indeed 👍. Currently in beer-mode, but I will have a look at this tomorrow 👍😊. My greatest concern here is the debugger. Have you tried debugging in VS Code using 100% isolation? |
|
I testet this just briefly today and it looks very promising indeed. Also ran a quick test to ensure debugging still works. This probably means that we can get rid of a couple of hacks like https://github.com/filipw/dotnet-script/blob/2c21516f1f42ab16712af32b365a4e1966c29ca5/src/Dotnet.Script.Core/Dotnet.Script.Core.csproj#L32 |
…o a more polished version of Gapotchenko.FX.Reflection.Loader.
|
This is awesome 👍 Just one more thing
|
Good point. Fixed. |
|
The following example documents the available levels of assembly isolation: Task<int> ExecuteScript(string filePath, IReadOnlyList<string> arguments)
{
var command = new ExecuteScriptCommand(ScriptConsole.Default, _LogFactory);
var scriptFile = new ScriptFile(filePath);
var commandOptions = new ExecuteScriptCommandOptions(scriptFile, arguments.ToArray(), OptimizationLevel.Release, null, false, false)
{
AssemblyLoadContext = CreateAssemblyLoadContext()
};
return command.Run<int, CommandLineScriptGlobals>(commandOptions);
}1. No assembly isolation: static AssemblyLoadContext? CreateAssemblyLoadContext() => null;2. Partial assembly isolation: sealed class IsolatedLoadContext : AssemblyLoadContext
{
protected override Assembly Load(AssemblyName assemblyName) => null;
}
static AssemblyLoadContext? CreateAssemblyLoadContext() => new IsolatedLoadContext();3. Full assembly isolation: static AssemblyLoadContext? CreateAssemblyLoadContext() => new ScriptAssemblyLoadContext();
4. Custom assembly isolation: static AssemblyLoadContext? CreateAssemblyLoadContext() => ...; // custom implementation of assembly load context |
|
@hrumhurum We just merged #633 which means that this PR needs to be updated to drop the |
|
The support for .NET Core 2.1 has been dropped. |
.NET Core deprecated the concept of app domains. Only one (main) AppDomain is allowed and it cannot contain the assemblies with different versions when their simple names clash.
When such a clash occurs, an attempt to load the assembly produces a specific exception:
When the script happens to reference an assembly which is referenced by the host, but with different version, it cannot be executed due to this error.
AssemblyLoadContextis the new way of assembly isolation that was introduced in .NET Core, and it allows to overcome the aforementioned limitation.This pull request will provide a gradual implementation of
AssemblyLoadContextsupport.Let's start with a technical refactoring that changes the affected interactions with
AppDomainclass toAssemblyLoadPal. That name stands for "Platform Abstraction Layer (PAL) for assembly loading functionality of a host environment" and comes from Gapotchenko.FX.Reflection.Loader module. The module contains other useful primitives as well but we leave them out for now.Everything
AssemblyLoadPaldoes is routes the assembly loading interactions withAppDomainto its own API. In this way, this tiny stable API can be reused when a host environment provides another ways of assembly loading such asAssemblyLoadContext. SoAssemblyLoadPalis a stable abstraction that transparently works over app domains, assembly load contexts, and maybe some other future ways of dealing with assemblies.In the first set of changes, the existing behavior of a script runner remains exactly the same but with the addition of that abstraction.
(To be continued)