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

Remove dependency from JSON.NET #2937

Closed
bradwilson opened this issue Oct 14, 2017 · 17 comments
Closed

Remove dependency from JSON.NET #2937

bradwilson opened this issue Oct 14, 2017 · 17 comments
Assignees
Labels
area-DependencyModel blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@bradwilson
Copy link

Today, you have a dependency on JSON.NET in some fundamental components (for example, Microsoft.Extensions.DependencyMode). This has the unfortunate side-effect that it becomes either difficult or impossible for end users to choose something newer without either running up against potential incompatibilities, or being outright unable to do so at all (for example, in a plugin model application which leverages M.E.D to load dependencies, but does so after the older JSON.NET has already been loaded).

An example of this dependency which becomes a hard-blocker is unit testing w/ xUnit.net, which uses M.E.D to load the dependencies of the unit test libraries. See: xunit/xunit#1060

I've previously discussed this with a few team members, but notably @DamianEdwards indicated that removing this in a point release (f.e. the upcoming 2.1) would be considered to be a breaking change. I'm not 100% sure I agree with that assessment, but either way, this is not a great situation for end users to be in, to have dependencies on things your team doesn't ship and doesn't control the revisions of.

@Petermarcu
Copy link
Member

outside of dependency model, I believe there are other parts of the ASP.NET stack that also have this dependency. Is it the case that if we don't remove all of them, then we still have the problem. I would like to see us free JSON.net from the platform although, we'd have to work through the details. We can probably move some of the platform components to use the low allocating Json parsers in corefx lab over time.

@bradwilson
Copy link
Author

There are two versions of this problem:

The "indirect" model

This is caused by applications that are indirectly consuming a library which has a competing version of a dependency (for example, the unit testing system in xUnit.net, or a plug-in system for an application). In this case the version linked by the originating application is loaded before you're even aware that a conflict may come up, so whatever version the application has already loaded will be the one that's used. The dynamically loaded code (f.e., the plug-in or the unit tests) get silently stuck with the application's preference.

In desktop CLR days, we had both app domains and cross-process RPC options that could be used to reduce or eliminate these problems, which xUnit.net does use (as probably do some applications that use plug-in models, if for no other reason than isolation). In .NET Core land, there is really nothing similar, because the platform has eliminated app domains, and AFAIK does not include any simple cross-process RPC, either (like old-school remoting).

The "direct" model

This is caused by applications which are directly consuming a library which has a competing version of a dependency (for example, an application which uses ASP.NET, and they have both expressed a dependency on JSON.NET). In this case you may get an auto-downgrade (with a compiler warning), or you may get an auto-upgrade (usually silently). Here, you're still stuck with a single version and the "losing" side may misbehave, just like in the "indirect" model. In many cases, the application is going to choose a newer version of the library, which means the auto-upgrade will be silent. Developers really have no mechanism to ensure that JSON.NET v10 (or 11, or 14...) is 100% compatible with ASP.NET, nor really any expectation that they've done anything "wrong" right up until the application breaks for unknown causes which are difficult to reason about.

Why this is worse for (ASP) .NET Core than everybody else

This is not a problem unique to ASP.NET Core, other than the fact that they have chosen a third party dependency on the single most used library in all of .NET (nearly triple the downloads of EF or ASP.NET MVC (non Core)). Any other library which ships for .NET (with NuGet or not, on .NET Core or not) has to contend with this being a potential issue. Ask anybody using ASP.NET Web API Client today (nearly 29 million downloads), with its dependency on JSON.NET v6, whether they can safely move to v10 or not.

Where it becomes exceptionally problematic is that the .NET Core Framework itself has taken the dependency, effectively freezing people in place until the framework takes on new versions of JSON.NET (which, by the way, is also a breaking change and therefore should only be done on major releases). Yes, ASP.NET Core and .NET Core are currently marching in lockstep, so that I guess those things are equivalent, but nobody expects them to rev major versions very often... I suspect it's measured in years, not months.

@JamesNK
Copy link
Member

JamesNK commented Oct 18, 2017

Isn't a simple solution here to replace JSON reading in Microsoft.Extensions.DependencyModel with the parser here? It was created because the old project.json was read using Newtonsoft.Json, so by the time the dependencies in project.json - possibly containing a different version of Newtonsoft.Json - were resolved Newtonsoft.Json was already loaded and you were stuck with the shipped version.

The issue with reading deps.json seems the same. If the "internal" Microsoft.Framework.Runtime parser was used by Microsoft.Extensions.DependencyModel then whatever version of Newtonsoft.Json is defined in deps.json is free to load.

@bradwilson
Copy link
Author

@JamesNK That's exactly what I ended up doing to remove the dependency in xUnit.net. We'll see if I stumble across files it can't parse, but given that the .deps.json file is intended to be generated and not hand crafted, I'm not concerned with issues like comment support.

https://github.com/xunit/xunit/blob/557e1d5cbf2c65fb33eafcbc5e439183a17406cc/src/common/Microsoft.Extensions.DependencyModel/DependencyContextJsonReader.cs

I'm sure from a performance and memory consumption perspective it's worse, but I'll work through those issues later if needed.

@eerhardt
Copy link
Member

@Petermarcu @terrajobst - There's a larger issue here than just DependencyModel. The issue is, if Newtonsoft.Json isn't "the" JSON library that should be used, then what is?

I've never seen an issue like this for XML where people are having issues because a library took a dependency on System.XML or System.XLinq.

I think we need to re-evaluate our JSON story all up if folks need to jump through hoops just to read/write JSON documents.

@Petermarcu
Copy link
Member

It sounds like the general sentiment is that Json.net should not be used by platforms or tools that shared the same context as applications. For those things, we need something else. Because this is part of a platform, it needs another option. This basically means we either need to provide a solution for Json parsing in the platform or have everything that needs it have its own private implementation.

We are looking into what it would look like to add efficient low level json parsing to .NET Core. That may align well. Alternatively, we could consider just moving to the one @JamesNK pointed to.

@DamianEdwards
Copy link
Member

We should talk more on this. I personally believe we need functional, performant JSON reading/writing in the platform (and eventually .NET Standard) and I'd like to start having serious discussions about the way forward on that.

@terrajobst
Copy link
Member

terrajobst commented Mar 16, 2018

Agreed on that. JSON.NET is the swiss army knife of JSON but we do need a low-level platform-provided thing as well. Performance is one reason, the other is dependency management and allowing higher levels to have more freedom.

@bradwilson
Copy link
Author

I've never seen an issue like this for XML where people are having issues because a library took a dependency on System.XML or System.XLinq .

That's because those thing ship with the platform. You get what you get by virtue of platform version. That's not the case with JSON.NET.

@DamianEdwards
Copy link
Member

DamianEdwards commented Mar 16, 2018

@Eilon @davidfowl

@Petermarcu
Copy link
Member

@DamianEdwards I am supportive of getting it into the platform.

@Petermarcu
Copy link
Member

Ok, so do we just let this issue hang out for a while while that gets sorted? @joshfree do we have an issue tracking adding json reader/writers to the platform?

@joshfree joshfree self-assigned this Mar 17, 2018
@Eilon
Copy link
Member

Eilon commented Mar 19, 2018

I'd love to be involved in the discussion.

@joshfree
Copy link
Member

joshfree commented Oct 30, 2018

@eerhardt
Copy link
Member

From my understanding of the above plan, the new JSON APIs will only be supported on netcoreapp3.0. If that is true, then DependencyModel will have to split its dependencies/implementation in order to take up these new APIs.

It is already multi-targeted today. We will just need to add a new netcoreapp3.0 TFM to the list, and remove the dependency on Newtonsoft for that TFM. Then implement the reading/writing of the .deps.json files with the new APIs on netcoreapp3.0.

@natemcmaster
Copy link
Contributor

Just FYI - Microsoft.Extensions.DependencyModel is one of the assemblies pulling Json.NET into the AspNetCore shared framework. I would consider this a blocking-RTM feature since it is our intention to remove Json.NET from the shared framework, and we can't remove our dependency on Microsoft.Extensions.DependencyModel

@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2019

This is now fixed for TFMs > netstandard2.0 by dotnet/core-setup#5009. Closing.

@eerhardt eerhardt closed this as completed Apr 3, 2019
@msftgits msftgits transferred this issue from dotnet/core-setup Jan 30, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-DependencyModel blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

No branches or pull requests

10 participants