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

Added ReferenceResolverProvider to JsonSerializer to allow safe caching #1393

Conversation

TylerBrinkley
Copy link
Contributor

@TylerBrinkley TylerBrinkley commented Jul 28, 2017

Fixes #1391
Fixes #1452
Fixes #870

@JamesNK
Copy link
Owner

JamesNK commented Aug 3, 2017

The reason I left JsonSerializer with a property that returns IReferenceResolver is the property can be used inside a JsonConverter's WriteJson/ReadJson (each has a JsonSerializer parameter). A func that returns a new instance each time will break people who want to use the resolver inside a converter.

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Aug 3, 2017

Hmm, could ReferenceResolver be added to JsonReader and JsonWriter instead which are both available to JsonConverters?

@JamesNK
Copy link
Owner

JamesNK commented Aug 4, 2017

No. References are specific to serialization.

…verter's access to the currently used IReferenceResolver.
@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Aug 4, 2017

It seems to me that JsonReaders and JsonWriters are not used concurrently and thus would work well to expose the currently used IReferenceResolver. As such I've updated the pull request to support that.

Basically, it's either this or JsonSerializers aren't safe to reuse if references are used.

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Aug 8, 2017

Hmm, maybe we could use a JsonSerializerProxy for each serialization call to pass to JsonConverters whose ReferenceResolver property is specific to that serialization call? Then we wouldn't need to add ReferenceResolver properties to JsonReader and JsonWriter and current JsonConverters will continue to just work. To ensure JsonSerializerProxy remains lightweight we could consider moving all of JsonSerializer's fields to a private settings class so that JsonSerializer only has one field which won't be used for JsonSerializerProxy.

…nSerializerProxy to use the internal serializer's ReferenceResolver.
@TylerBrinkley
Copy link
Contributor Author

I just implemented what I proposed above. It looks like a JsonSerializerProxy was already being passed to JsonConverters so no changes needed there.

@TylerBrinkley
Copy link
Contributor Author

TylerBrinkley commented Aug 8, 2017

I just realized you baked your DefaultReferenceResolver's state into JsonSerializerInternalBase's DefaultReferenceMappings property. The only state that is not baked in is the reference count hence the creation of #1391. As opposed to baking the reference count into JsonSerializerInternalBase it seems like the right thing to do in this situation would be to move DefaultReferenceMappings into the resolver itself and use the changes I have made. This way custom IReferenceResolvers will work just as well as the DefaultReferenceResolver.

This has been implemented in the PR.

@TylerBrinkley
Copy link
Contributor Author

Resolved merge conflicts

@TylerBrinkley
Copy link
Contributor Author

@JamesNK Please review when you have the time. Thanks.

@Terebi42
Copy link

I am experiencing this bug in production, please accept this PR

@dtila
Copy link

dtila commented Sep 7, 2022

This PR does not fixes the issue, so don't merge.
There is still an issue with the threading and the bidirectional mapping. The first equality comparer is the default, and the second is a reference only. For class files, it will causes an unbalanced dictionary for classes implementing IEqutable.

@TylerBrinkley
Copy link
Contributor Author

Closing due to age.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants