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

DefaultReferenceResolver Concurrency Fix #2170

Closed
wants to merge 2 commits into from

Conversation

TylerBrinkley
Copy link
Contributor

In case you won't accept #1393 this is the easy fix to some of the concurrency issues.

Fixes #870 and #1452

@TylerBrinkley TylerBrinkley changed the title Update DefaultReferenceResolver.cs DefaultReferenceResolver Concurrency Fix Sep 20, 2019
@Aaronontheweb
Copy link

Any update on this? Ran into this issue in akkadotnet/akka.net#5197

@JamesNK
Copy link
Owner

JamesNK commented Aug 14, 2021

This type isn't designed to have thread-safety.

Even if the id was incremented with Interlocked, the underlying collections aren't thread-safe. And if the collections were made threadsafe then performance would probably take a big hit because all serialization shares the same collections and there will be a ton of lock contention.

@TylerBrinkley
Copy link
Contributor Author

It seems the underlying BidirectionalDictionary is specific to the serialization context and thus does not need to be thread safe as it should only be accessed from a single thread whereas the DefaultReferenceResolver itself is shared across serialization calls thus users have experienced these concurrency issues.

You yourself said JsonSerializer is thread-safe at https://stackoverflow.com/questions/36186276/is-the-json-net-jsonserializer-threadsafe so it seems the intent would be for the DefaultReferenceResolver to be thread-safe also since it's used by JsonSerializer but that's not true when using references as indicated in my answer to that question. This small change should fix the concurrency issue but I still think this type should either avoid being stateful by pushing the reference count into the context or else creating a new ReferenceResolver for each serialization call as is proposed in #1393.

@Aaronontheweb
Copy link

Yeah the idea that the serializer itself isn't / can't be thread-safe seems crazy to me. Move the state to the context or the call, not the callsite itself.

@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
Development

Successfully merging this pull request may close these issues.

Concurrency issue in DefaultReferenceResolver/JsonSerializer
3 participants