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.

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

Assign PropertyCache after it's configured for source gen in attempt to fix JSON LookupProperty assert failing #68480

Closed
wants to merge 1 commit into from
Closed

Assign PropertyCache after it's configured for source gen in attempt to fix JSON LookupProperty assert failing #68480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,19 +220,23 @@ internal virtual void Configure()
// also assigns JsonPropertyInfo's JsonTypeInfo which causes SO if there are any
// cycles in the object graph. For that reason properties cannot be added immediately.
// This is a no-op for ReflectionJsonTypeInfo
LateAddProperties();
var propertyCache = LateAddProperties() ?? PropertyCache;

DataExtensionProperty?.EnsureConfigured();

if (converter.ConverterStrategy == ConverterStrategy.Object && PropertyCache != null)
if (converter.ConverterStrategy == ConverterStrategy.Object && propertyCache != null)
{
foreach (var jsonPropertyInfoKv in PropertyCache.List)
foreach (var jsonPropertyInfoKv in propertyCache.List)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should the declaring type's NumberHandling be passed to EnsureConfigured() as an argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

more likely this (declaring type JsonTypeInfo) perhaps because we don't really know if other info might be needed here. I'll refrain from changing it in this PR

{
JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value!;
jsonPropertyInfo.DeclaringTypeNumberHandling = NumberHandling;
jsonPropertyInfo.EnsureConfigured();
}

// This code can be run in multiple threads therefore we assign only after all properties has been configured
Copy link
Member

@eiriktsarpalis eiriktsarpalis Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this change makes the code more complicated without addressing the core issue, which is described in the comment here. It doesn't fix all potential race conditions hiding in the initialization code, nor does it prevent any future races from being introduced as the code gets changed.

The only real solution is to prevent multiple threads from attempting to write to an instance. There are a few approaches we could pursue longer term, but the immediate need is to unblock CI for others. As an interim solution I would recommend adding a double-checked lock in the EnsureConfigured() method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hoping we could achieve same without having to do locks but agreed it will make it harder to prevent from the regression to happen again. Will revert and lock

Copy link
Member Author

Choose a reason for hiding this comment

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

actually let me close this PR and create a new one since it's effectively different change

// This only matters in case we call LateAddProperties
PropertyCache = propertyCache;

if (converter.ConstructorIsParameterized)
{
InitializeConstructorParameters(GetParameterInfoValues(), sourceGenMode: Options.JsonSerializerContext != null);
Expand Down Expand Up @@ -280,7 +284,7 @@ internal string GetDebugInfo()
}
#endif

internal virtual void LateAddProperties() { }
internal virtual JsonPropertyDictionary<JsonPropertyInfo>? LateAddProperties() { return null; }

internal virtual JsonParameterInfoValues[] GetParameterInfoValues()
{
Expand Down
18 changes: 9 additions & 9 deletions .../System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ private static JsonConverter GetConverter(JsonCollectionInfoValues<T> collection
return new JsonMetadataServicesConverter<T>(converterCreator, strategy);
}

internal override void LateAddProperties()
internal override JsonPropertyDictionary<JsonPropertyInfo>? LateAddProperties()
{
AddPropertiesUsingSourceGenInfo();
return AddPropertiesUsingSourceGenInfo();
}

internal override JsonParameterInfoValues[] GetParameterInfoValues()
Expand All @@ -110,11 +110,11 @@ internal override JsonParameterInfoValues[] GetParameterInfoValues()
return array;
}

internal void AddPropertiesUsingSourceGenInfo()
internal JsonPropertyDictionary<JsonPropertyInfo>? AddPropertiesUsingSourceGenInfo()
{
if (PropertyInfoForTypeInfo.ConverterStrategy != ConverterStrategy.Object)
{
return;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also exit early for typeof(T)?

Copy link
Member Author

Choose a reason for hiding this comment

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

likely yes but I'd prefer to keep those changes separate

}

JsonSerializerContext? context = Options.JsonSerializerContext;
Expand All @@ -123,23 +123,23 @@ internal void AddPropertiesUsingSourceGenInfo()
{
if (typeof(T) == typeof(object))
{
return;
return null;
}

if (PropertyInfoForTypeInfo.ConverterBase.ElementType != null)
{
// Nullable<> or F# optional converter's strategy is set to element's strategy
return;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there some Debug.Asserts that would be relevant to verify that this is a Nullable or F# optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

This specific check is happening because Nulalble<T> converter currently reports Object as its converter strategy but it actually never uses properties - IMO this is incorrect but I didn't want to do the needed refactoring to change - I was preserving existing behavior here. I'll refrain from doing this change in this PR

Copy link
Member

Choose a reason for hiding this comment

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

This specific check is happening because Nulalble converter currently reports Object as its converter strategy but it actually never uses properties

It only will report Object if T is a struct. In any case it would make sense to an assertion here in case the ElementType assumption gets invalidated by a different converter in the future.

}

if (SerializeHandler != null && Options.JsonSerializerContext?.CanUseSerializationLogic == true)
{
ThrowOnDeserialize = true;
return;
return null;
}

ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(context, Type);
return;
return null;
}

Dictionary<string, JsonPropertyInfo>? ignoredMembers = null;
Expand Down Expand Up @@ -179,7 +179,7 @@ internal void AddPropertiesUsingSourceGenInfo()
CacheMember(jsonPropertyInfo, propertyCache, ref ignoredMembers);
}

PropertyCache = propertyCache;
return propertyCache;
}

private void SetCreateObjectFunc(Func<T>? createObjectFunc)
Expand Down