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

Ensure serializer reflection-dependency sentinel is accurate #66522

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Mar 11, 2022

Fixes #66469.

@layomia layomia added this to the 7.0.0 milestone Mar 11, 2022
@layomia layomia self-assigned this Mar 11, 2022
@ghost
Copy link

ghost commented Mar 11, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #66469.

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json

Milestone: 7.0.0

if (s_defaultSimpleConverters is null)
// s_typeInfoCreationFunc is the last field assigned.
// Use it as the sentinel to ensure that all dependencies are initialized.
if (s_typeInfoCreationFunc is null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about his a little bit more, and you may want to use Volatile.Read(ref ...) here as well. The reason is that we don't want an optimizing compiler / CPU to attempt to read s_defaultSimpleConverters or s_defaultFactoryConverters before s_typeInfoCreationFunc is confirmed not-null. Using Volatile.Read here to complement the Volatile.Write you have below will help guarantee this can never happen on the read side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we might want to service 6.0 with this change since that particular change was cherry picked in #65898.

@layomia layomia merged commit 8255f56 into dotnet:main Mar 15, 2022
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…66522)

* Ensure serializer reflection-dependency sentinel is accurate

* Address feedback
@ghost ghost locked as resolved and limited conversation to collaborators Apr 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json.Tests crashing from Debug.Assert in JsonSerializerOptions.GetJsonTypeInfoFromContextOrCreate
3 participants