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

Unable to reference Newtonsoft.Json > 11.0.2 #2606

Closed
pritchums opened this issue Aug 19, 2019 · 19 comments

Comments

@pritchums
Copy link
Contributor

commented Aug 19, 2019

What You Are Seeing?

When referencing Newtonsoft.Json > 11.0.2, Cake fails when loading assembly. This is because Cake.Core (netstandard 2.0 only) and Cake.Nuget have a dependency on Newtonsoft.Json 11.0.2, which it loads during execution.

This is the same issue as captured under #2116, however the changes made to the resolver do not appear to work, as Cake is trying to load the assemblies based on absolute path.

I had previously submitted #2597 to pull in the latest version of Newtonsoft.Json at point of build, but this was rejected as it did not pin to a specific version (a point I concede).

I propose to update Cake.Core.csproj and Cake.Nuget.csproj to point directly to latest version of Newtonsoft.Json, which is, at the time of writing, 12.0.2.

What is Expected?

Assembly to be loaded without error

What version of Cake are you using?

0.34.1

Are you running on a 32 or 64 bit system?

64

What environment are you running on? Windows? Linux? Mac?

Windows & Linux, using dotnet core tool
Does not appear to affect framework version of Cake, as this does not appear to load Newtonsoft.Json during normal execution.

Are you running on a CI Server? If so, which one?

N/A - issue occurs locally too.

How Did You Get This To Happen? (Steps to Reproduce)

Add the following to build.cake:
#addin nuget:?package=Newtonsoft.Json&version=12.0.2

Output Log

Output:

Error: System.IO.FileLoadException: Could not load file or assembly 'Newtonsoft.Json, Version=12.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed'.
   at System.Runtime.Loader.AssemblyLoadContext.LoadFromPath(IntPtr ptrNativeAssemblyLoadContext, String ilPath, String niPath, ObjectHandleOnStack retAssembly)
   at System.Runtime.Loader.AssemblyLoadContext.LoadFromAssemblyPath(String assemblyPath)
   at System.Reflection.Assembly.LoadFrom(String assemblyFile)
   at Cake.Core.Reflection.AssemblyLoader.Load(FilePath path, Boolean verify) in C:\projects\cake\src\Cake.Core\Reflection\AssemblyLoader.cs:line 31
   at Cake.Core.Scripting.ScriptRunner.Run(IScriptHost host, FilePath scriptPath, IDictionary`2 arguments) in C:\projects\cake\src\Cake.Core\Scripting\ScriptRunner.cs:line 171
   at Cake.Commands.BuildCommand.Execute(CakeOptions options) in C:\projects\cake\src\Cake\Commands\BuildCommand.cs:line 41
   at Cake.CakeApplication.Run(CakeOptions options) in C:\projects\cake\src\Cake\CakeApplication.cs:line 45
   at Cake.Program.Main() in C:\projects\cake\src\Cake\Program.cs:line 73
@pritchums

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@gep13 - issue as requested

@gep13

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@pritchums thank you for adding this, I appreciate it!

@mholo65 @daveaglick can either of you offer anything here?

While updating the Newtonsoft.Json PackageReference to 12.0.2 will likely fix the issue, I suspect that it will just happen again when a newer version comes out. Right?

@daveaglick

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

however the changes made to the resolver do not appear to work, as Cake is trying to load the assemblies based on absolute path

It took me a minute to grok what's going on here:

  • The ScriptAssemblyResolver mentioned above hooks AppDomain.CurrentDomain.AssemblyResolve which only gets called if an assembly load is attempted by name and fails:
    private Assembly AssemblyResolve(object sender, ResolveEventArgs args)
  • ...however, the AssemblyHelper called by AssemblyLoader only attempts a load by name if the assembly path isn't valid:
    if (path.Segments.Length == 1 && !fileSystem.Exist(path))
  • In this case ScriptRunner finds and loads references by path, which is why the binding fix in ScriptAssemblyResolver never comes into play:
    // Load all references.
    var applicationRoot = _environment.ApplicationRoot;
    var assemblies = new HashSet<Assembly>();
    assemblies.AddRange(_conventions.GetDefaultAssemblies(applicationRoot));
    foreach (var reference in result.References)
    {
    var referencePath = new FilePath(reference);
    if (host.Context.FileSystem.Exist(referencePath))
    {
    var assembly = _assemblyLoader.Load(referencePath, true);
    assemblies.Add(assembly);
    }
    else
    {
    // Add a reference to the session.
    session.AddReference(referencePath);
    }
    }
  • ...and because Cake.Core references Newtonsoft.Json 11.0.2, and that reference is already in the AppDomain when ScriptRunner tries to load an alternate version by path, we never redirect the binding to the already loaded version and BOOM!

Updating to the latest version of Newtonsoft.Json will fix the issue short-term - until a newer version of Newtonsoft.Json comes out and someone tries to reference that as an addin. To fix this long-term, we'll need a similar code-based binding redirection like we did for the ScriptAssemblyResolver inside either ScriptRunner (or probably better in AssemblyHelper).

My first instinct that we should replace this line with an Assembly.ReflectionOnlyLoadFrom() first, check if the current app domain contains an assembly with the same name but different version, and then skip the actual Assembly.LoadFrom() if so. That'll still cause runtime problems if the script is expecting and uses features from a newer version than what's already loaded, but at least it won't crash and will hopefully work in most cases. We should also still update Newtonsoft.Json to avoid as many runtime missing method problems as possible though.

@mholo65 can you check my work and assumptions here?

@patriksvensson

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@daveaglick There is no AppDomain on .NET Core so it won't solve the problem there.
Also, does Cake.Core really reference Newtonsoft.Json?

@daveaglick

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@patriksvensson Yeah, poor choice of words. I think we'll need to check the AssemblyLoadContext equivalent for currently loaded assemblies when deciding to load a same-assembly-but-different-version in the AssemblyHelper.

@mholo65

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@daveaglick said:

My first instinct that we should replace this line with an Assembly.ReflectionOnlyLoadFrom() first, check if the current app domain contains an assembly with the same name but different version, and then skip the actual Assembly.LoadFrom() if so.

I actually tried that in #2564 as I had problems loading System.Diagnostics.DiagnosticSource as it was already referenced by the app (via NetCoreApp IIRC). However I couldn’t find the assembly when searching loaded assemblies. I added a Try/Catch instead and ”it works...”.

@daveaglick

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

”it works...”

😬

@patriksvensson

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@gep13 @mholo65 @daveaglick Why is that assembly being referenced there? Does anyone know?

@mholo65

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

You know those times when you debug an App in VS and the debug window is flooded with ”Could not load Assembly XYZ...” but the app still works... 😄

@devlead

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@gep13 @mholo65 @daveaglick Why is that assembly being referenced there? Does anyone know?

Because NuGet client libs

@patriksvensson

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@gep13 @mholo65 @daveaglick Why is that assembly being referenced there? Does anyone know?

Because NuGet client libs

I see no reason why it should be in Cake.Core because of Cake.NuGet?

@mholo65

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@gep13 @mholo65 @daveaglick Why is that assembly being referenced there? Does anyone know?

Because NuGet client libs

I see no reason why it should be in Cake.Core because of Cake.NuGet?

git blame? Sorry not at computer.

@pritchums

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@gep13 @mholo65 @daveaglick Why is that assembly being referenced there? Does anyone know?

Because NuGet client libs

I see no reason why it should be in Cake.Core because of Cake.NuGet?

git blame? Sorry not at computer.

It's in Cake.Core, because the Microsoft.Extensions.DependencyModel 2.0.4 is referenced for netstandard, and this references Newtonsoft.Json >= 9.0.1

(and Cake.NuGet via NuGet.Packaging)

@pritchums

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Hi all. Just wondering what your current thoughts are on this?

The Cake.Core dependency on Microsoft.Extensions.DependencyModel looks superfluous to me, given Cake uses Autofac. The version history suggests this was added when Cake was originally modified to support dotnet core, and I suspect it was a MS default addition. Rider/ReSharper suggests the reference is unused, and (with a very simple test case) Cake appears to still function under dotnet core without this reference.

Removing the Cake.Core dependency on Microsoft.Extensions.DependencyModel would allow the Cake.Core dependency on Newtonsoft.Json to also be removed, but the Cake.NuGet dependency on Newtonsoft.Json will need remain due to the dependency on NuGet.Packaging.

It sounds like you have some good ideas to improve assembly loading, but would it be worth breaking out that improvement into a separate issue? While work on that issue in progress, we could update the existing references to Newtonsoft.Json to latest (12.0.2), so anyone using cake on dotnet core can reference packages using Newtonsoft.Json > 11.0.2.

@mholo65

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Microsost.Extensions.DependencyModel contains abstractions for reading .deps files and AFAIK nothing that we need. Could be removed IMO.

@pritchums

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Thanks @mholo65

Any objections if I submit a single PR removing the Cake.Core dependency on Microsoft.Extensions.DependencyModel, and updating Newtonsoft.Json in Cake.NuGet to 12.0.2?

@pritchums

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

Hi @devlead @gep13 @mholo65 @daveaglick
Wondering what your thoughts are on adding PR #2615 to the 0.35.0 milestone? This is currently a blocker for us, as one of our dependencies annoyingly requires Newtonsoft.Json 12.0.2.

@devlead devlead added the Improvement label Sep 28, 2019
@devlead devlead added this to the v0.35.0 milestone Sep 28, 2019
@devlead

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Fixed by #2597

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.