Speclet for task isolation and dependency resolution #4133
Conversation
Initial description of * Isolating tasks to avoid load failures * Task dependency resolution based on `.deps.json`
| This problem is also described in microsoft/msbuild#1754. | ||
|
|
||
| ## Possible solution | ||
| Use [`AssemblyLoadContext`](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext?view=netcore-2.2) (ALC) to provide binding isolation for tasks. Each task would be loaded into its own ALC instance. |
There was a problem hiding this comment.
Should this be at task (class) or assembly granularity? The whole assembly would have to have the same dependency closure, right?
There was a problem hiding this comment.
Definitely assembly level - I'll change the wording..
| Use [`AssemblyLoadContext`](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext?view=netcore-2.2) (ALC) to provide binding isolation for tasks. Each task would be loaded into its own ALC instance. | ||
| * The ALC would resolve all dependencies of the tasks (see dependency resolution below) | ||
| * ALC would fallback to the Default for dependencies which the task doesn't carry with itself (Framework and so on) | ||
| * ALC would probably have to forcefully fallback for MSBuild assemblies since by default lot of tasks will carry these, but the system requires for the MSBuild assemblies to be shared. |
There was a problem hiding this comment.
What does "carry" mean in this context? I wouldn't expect tasks to redistribute MSBuild assemblies, but they will depend on them. How does that impact their .deps.json.
There was a problem hiding this comment.
It means the task has a second copy of some of the MSBuild assemblies with it. I agree that well behaved tasks should not do this. The problem is tooling defaults. If one creates a classlib and builds it, all the dependencies are copied into the output. There are ways to mark the dependencies as "don't copy to output", but it's something which has to be done explicitly. If somebody forgets, the assembly will be in the output. We've ran into this with "plugin load" samples and it's relatively hard to diagnose when that happens. It's better to try to avoid this in the host if possible.
|
|
||
| ## Potential risks | ||
| * Has some small probability of causing breaks. Currently all assemblies from all tasks are loaded into the default context and thus are "visible" to everybody. Tasks with following properties might not work: | ||
| * Task has a dependency on an assembly, but it doesn't declare this dependency in its .deps.json and this dependency gets loaded through some other task. This is mostly fixable by implementing probing similar to today's behavior. |
There was a problem hiding this comment.
I do think we'll have to have fallback behavior exactly as we do today to keep current tasks working.
There was a problem hiding this comment.
I agree it might be a good idea to add some way to "disable" the isolation as a workaround for "broken" tasks.
| ## Potential risks | ||
| * Has some small probability of causing breaks. Currently all assemblies from all tasks are loaded into the default context and thus are "visible" to everybody. Tasks with following properties might not work: | ||
| * Task has a dependency on an assembly, but it doesn't declare this dependency in its .deps.json and this dependency gets loaded through some other task. This is mostly fixable by implementing probing similar to today's behavior. | ||
| * Two tasks from different assemblies which somehow rely on sharing certain types. If the new system decides to load these in isolation they won't share types anymore and might not work. |
There was a problem hiding this comment.
My understanding is that this is possible but difficult in MSBuild and I don't think I've actually seen a case where it's done, so this seems ok.
|
|
||
| ## Additional consideration | ||
| * None of these changes would have any effect on MSBuild on .NET Framework | ||
| * Task isolation alone could be achieved on existing MSBuild |
There was a problem hiding this comment.
With an ALC but no .deps.json handling?
There was a problem hiding this comment.
I think it would be nice if deps.json were optional to get ALC isolation. Can custom ALC behave as SetAppPaths option in runtimeconfig for default ALC / app?
There was a problem hiding this comment.
If deps.json files is missing, the resolver works with the contents of the published directory of the task.
There was a problem hiding this comment.
To answer @rainersigwald question: yes, we could add isolation logic into MSBuild even on .NET Core 2.*. MSBuild seems to have complex enough probing logic to make most tasks work (find their dependencies). The isolation would be about solving problems with tasks which have incompatible dependencies.
.deps.json handling makes the whole thing even more robust and enables additional scenarios, but it requires 3.0 APIs.
| It was designed to be used as the underlying piece to implement custom ALC. So it would work nicely with task isolation above. | ||
|
|
||
| ## Potential risks | ||
| * Small probability of breaking tasks which have `.deps.json` with them and those are not correct. With this change the file would suddenly be used and coudl cause either load failures or different versions of assemblies to get loaded. |
There was a problem hiding this comment.
| * Small probability of breaking tasks which have `.deps.json` with them and those are not correct. With this change the file would suddenly be used and coudl cause either load failures or different versions of assemblies to get loaded. | |
| * Small probability of breaking tasks which have `.deps.json` with them and those are not correct. With this change the file would suddenly be used and could cause either load failures or different versions of assemblies to get loaded. |
| It was designed to be used as the underlying piece to implement custom ALC. So it would work nicely with task isolation above. | ||
|
|
||
| ## Potential risks | ||
| * Small probability of breaking tasks which have `.deps.json` with them and those are not correct. With this change the file would suddenly be used and coudl cause either load failures or different versions of assemblies to get loaded. |
There was a problem hiding this comment.
This seems ok since it'll go with the major 3.0 release.
| Tasks with complex and specifically platform specific dependencies don't work out of the box. For example if a task uses [`LibGit2Sharp`](https://www.nuget.org/packages/LibGit2Sharp) package it will not work as is. `LibGit2Sharp` has native dependencies which are platform specific. While the package carries all of them, there's no built in support for the task to load the right ones. For example [source link](https://github.com/dotnet/sourcelink/blob/master/src/Microsoft.Build.Tasks.Git/GitLoaderContext.cs) runs into this problem. | ||
|
|
||
| ## Possible solution | ||
| .NET Core uses `.deps.json` files to describe dependencies of components. It would be natural to treat task assemblies as components and use associated .deps.json file to determine their dependencies. This would make the system work nicely end to end with the .NET Core CLI/SDK and VS integration. |
There was a problem hiding this comment.
What is the authoring experience for .deps.json for "plugin" DLLs. What will a task author need to do on their end to make this work correctly?
There was a problem hiding this comment.
We generate one today for all .net core projects. But I filed dotnet/sdk#2662
There was a problem hiding this comment.
As @nguerrera mentioned above, today it's enough to dotnet new classlib and dotnet build (or dotnet publish).
| Tasks with complex and specifically platform specific dependencies don't work out of the box. For example if a task uses [`LibGit2Sharp`](https://www.nuget.org/packages/LibGit2Sharp) package it will not work as is. `LibGit2Sharp` has native dependencies which are platform specific. While the package carries all of them, there's no built in support for the task to load the right ones. For example [source link](https://github.com/dotnet/sourcelink/blob/master/src/Microsoft.Build.Tasks.Git/GitLoaderContext.cs) runs into this problem. | ||
|
|
||
| ## Possible solution | ||
| .NET Core uses `.deps.json` files to describe dependencies of components. It would be natural to treat task assemblies as components and use associated .deps.json file to determine their dependencies. This would make the system work nicely end to end with the .NET Core CLI/SDK and VS integration. |
There was a problem hiding this comment.
We generate one today for all .net core projects. But I filed dotnet/sdk#2662
| * Performance - task isolation inherently (and by design) leads to loading certain assemblies multiple times. This increases memory pressure and causes additional JITing and other related work. | ||
|
|
||
| ## Additional consideration | ||
| * None of these changes would have any effect on MSBuild on .NET Framework |
There was a problem hiding this comment.
I dream of being able to produce a netstandard msbuild task assembly that can work on .NET framework or .NET core, but I get that this would be out of scope. The hard problem there is when your dependencies have different implementations for each.
|
|
||
| ## Additional consideration | ||
| * None of these changes would have any effect on MSBuild on .NET Framework | ||
| * Task isolation alone could be achieved on existing MSBuild |
There was a problem hiding this comment.
I think it would be nice if deps.json were optional to get ALC isolation. Can custom ALC behave as SetAppPaths option in runtimeconfig for default ALC / app?
| # Task isolation | ||
| ## Problem definition | ||
| Tasks in MSBuild are dynamically loaded assemblies with potentially separate and colliding dependency trees. Currently MSBuild on .NET Core has no isolation between tasks and as such only one version of any given assembly can be loaded. Prime example of this is Newtonsoft.Json which has multiple versions, but all the tasks must agree on it to work. | ||
| This problem is also described in microsoft/msbuild#1754. |
There was a problem hiding this comment.
| This problem is also described in microsoft/msbuild#1754. | |
| This problem is also described in #1754. |
| @@ -0,0 +1,38 @@ | |||
| # Task isolation | |||
| ## Problem definition | |||
There was a problem hiding this comment.
Will the isolation be enforced for all tasks? Or is this an abstraction that will be exposed by MSBuild so that tasks (or groups of tasks) can opt in/out wrt isolation?
There was a problem hiding this comment.
That's unclear. I personally think it should be "enforced". Tasks which don't do weird things should not notice. The only downside is performance, the upsides are that things just work.
There was a problem hiding this comment.
I tend to agree. We should probably add a global escape hatch switch just in case something does go wrong, but I think the isolation is basically what a user would hope for/expect without having to know the details.
There was a problem hiding this comment.
Given the "one version per ALC", I think it's basically broken to not isolate. On framework, each task could at least load its dependencies of different versions (when strong named), even without opting in to app domain isolation. I don't think we should allow an opt out for ALC. We can see if there's a real problem (perf or otherwise) in 3.0 feedback and add opt out if there's a compelling case.
There was a problem hiding this comment.
Will this mechanism prevent tasks from seeing static state set by other tasks? I assume not (although that would be a good thing, I am pretty sure there are tasks that use statics for caching)
There was a problem hiding this comment.
@danmosemsft that depends what assemblies are loaded into the default load context. Currently the assumption is only those needed by MSBuild itself. So if those have static state, that would be shared across all tasks (and visible).
Tasks coming from the same assembly would also see the same static state (the isolation is on assembly level, not task level).
Tasks from different assemblies would not see each others static state, even if they use the same assembly dependency as those would be duplicated/isolated.
There was a problem hiding this comment.
I think that is basically what we want.
Ignoring hacks, I think there is one proper pattern where task assemblies can store and share static state in MSBuild itself:
Today you could share say a parsed NewtonSoft.Json tree between task assemblies using this. However, I've really only seen task assemblies put their own data into this without expecting other task assemblies to use it. And I've even seen bugs where people copy and paste code from one task assembly into another and get InvalidCastException trying to pull things out of this API that they think they put in, but in reality it was another task assembly.
We should probably consider isolating this state as well by changing the backing dictionaries to be unique per ALC / task assembly.
| * The ALC would resolve all dependencies of the tasks (see dependency resolution below) | ||
| * ALC would fallback to the Default for dependencies which the task doesn't carry with itself (Framework and so on) | ||
| * ALC would probably have to forcefully fallback for MSBuild assemblies since by default lot of tasks will carry these, but the system requires for the MSBuild assemblies to be shared. | ||
| We would probably also want to load groups of tasks which belong together into the same ALC (for example based on their location on disk) to improve performance. This will need some care as there's no guarantee that two random tasks have compatible dependency trees. |
There was a problem hiding this comment.
I don't think MSBuild should carry policy (ex: path) to load tasks into the same ALC. If isolation contexts are exposed, then this can be handled correctly.
There was a problem hiding this comment.
This is potential performance optimization. I would prefer for the isolation to be mostly transparent to tasks. I worry that if tasks can somehow participate in isolation decisions, they will become much less likely to work in random combinations.
|
@rainersigwald if you think it's time, please merge this (I don't have right on this repo to merge into it). |
Initial description of
.deps.json