Please add documentation for thread-safety guarantees of JsonDocument (and related types)

[dpaoliello]

Description

Looking at the documentation for JsonDocument it is not obvious if it can be safely used by multiple threads or not.

A cursory glance at the code suggests that the parsing is done up-front and, therefore, it is safe - however we tripped over the caching that is done via _lastIndexAndString (i.e., if multiple threads call GetString on an element at the same time, then they may get back the wrong value).

I want to be clear that this is a doc bug: I'm fine with having a string cache (since I assume that it provides noticeable perf benefits), but I would have liked clear guidance that JsonDocument and uses of any JsonElement from the same parent document cannot be done safely from multiple threads (without calling Clone on each element).

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: 588ae32f-3429-143f-824c-5ab2d046d24a
• Version Independent ID: 002f9f2a-48d1-0671-a079-b99b9f6bab58
• Content: How to serialize and deserialize JSON using C# - .NET
• Content Source: docs/standard/serialization/system-text-json-how-to.md
• Product: dotnet
• GitHub Login: @tdykstra
• Microsoft Alias: tdykstra

[layomia]

FYI @tdykstra - hope to address this in the .NET 5 doc wave for JSON.

[kstittleburg]

I don't think documentation was adequately updated for this issue, ran into this in some code at my day job. With the fix PR (#24938) it now explicitly states JsonDocument is thread-safe and only mentions cloning in the context of object disposal, when the original issue is that JsonDocument (and by extension JsonElement or JsonProperty that utilize JsonDocument) are not thread safe (edit: or at least my definition of what thread-safe means) due to _lastIndexAndString caching.

[adj123]

Seconded, please amend this. I also just wasted days tracing a sporadic concurrency bug in my application code down to an immutable BCL type which is documented as thread-safe.

[tdykstra]

@eiriktsarpalis @gewarren Please see the latest two comments on this issue.

[eiriktsarpalis]

however we tripped over the caching that is done via _lastIndexAndString (i.e., if multiple threads call GetString on an element at the same time, then they may get back the wrong value).

Looking at the code, I would guess that this can only happen because of torn reads/writes. It might be that we could fix this in the code using a regular System.Tuple, thus ensuring atomic access to cached entries. Obviously this has the downside of incurring one additional allocation, but at least we can guarantee immutability (modulo disposability of course).

Related to dotnet/runtime#17975

cc @layomia @krwq

[eiriktsarpalis]

We probably don't want to sacrifice performance (added allocations) for thread safety, so until we have atomic APIs for 128-bit data we should probably just document that the type is not thread safe.

[adj123]

Thanks for looking at this so speedily after our revival of this issue. I'm not sure without further clarification of your intent that it can be said to be closed though, as the PR only removes the mention of thread safety without explicitly stating that it is not thread safe, and thus we're just back to the same situation we were in when this issue was first raised - the OP requested:

but I would have liked clear guidance that JsonDocument and uses of any JsonElement from the same parent document cannot be done safely from multiple threads (without calling Clone on each element).

[eiriktsarpalis]

Copying my response from the latest PR:

For all intents and purposes the design of JsonDocument was to provide an immutable and thus thread-safe DOM representation for JSON values. What impacts thread safety is in fact a concurrency bug impacting specific methods of the API that we should be fixing (and potentially servicing in older releases).

I'm not sure how to best express this in documentation other noting it as a known bug.