Skip to content

Ensure serializer reflection-dependency sentinel is accurate #66522

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.

Sign up for GitHub

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

Merged
layomia merged 2 commits into from
Mar 15, 2022
Merged

Ensure serializer reflection-dependency sentinel is accurate #66522

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ac29b0
Ensure serializer reflection-dependency sentinel is accurate
layomia Mar 11, 2022
8547ab3
Address feedback
layomia Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Converters;
using System.Text.Json.Serialization.Metadata;
using System.Threading;

namespace System.Text.Json
{
Expand All @@ -29,11 +30,14 @@ public sealed partial class JsonSerializerOptions
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
private static void RootReflectionSerializerDependencies()
{
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

@GrabYourPitchforks GrabYourPitchforks Mar 12, 2022

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

@eiriktsarpalis eiriktsarpalis Mar 14, 2022

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.

{
s_defaultSimpleConverters = GetDefaultSimpleConverters();
s_defaultFactoryConverters = GetDefaultFactoryConverters();
s_typeInfoCreationFunc = CreateJsonTypeInfo;
// Explicitly ensure that the previous fields are initialized along with this one.
Volatile.Write(ref s_typeInfoCreationFunc, CreateJsonTypeInfo);
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
Expand Down
4 changes: 3 additions & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,9 @@ private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
return null!;
}

Debug.Assert(s_typeInfoCreationFunc != null);
Debug.Assert(
s_typeInfoCreationFunc != null,
"Reflection-based JsonTypeInfo creator should be initialized if IsInitializedForReflectionSerializer is true.");
return s_typeInfoCreationFunc(type, this);
}

Expand Down