Newtonsoft.Json icon indicating copy to clipboard operation
Newtonsoft.Json copied to clipboard

DefaultReferenceResolver Concurrency Fix

Open TylerBrinkley opened this issue 6 years ago • 4 comments

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

Fixes #870 and #1452

TylerBrinkley avatar Sep 20 '19 19:09 TylerBrinkley

Any update on this? Ran into this issue in https://github.com/akkadotnet/akka.net/pull/5197

Aaronontheweb avatar Aug 12 '21 23:08 Aaronontheweb

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.

JamesNK avatar Aug 14 '21 23:08 JamesNK

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.

TylerBrinkley avatar Aug 16 '21 14:08 TylerBrinkley

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.

Aaronontheweb avatar Aug 16 '21 14:08 Aaronontheweb