Investigate thread safety of GetReadOnlyTypeInfo and SystemTextJsonOutputFormatter

[halter73]

It came up in API review for MakeReadOnly(bool populateMissingResolver) that the pattern of conditionally setting JsonSerializerOptions.TypeInfoResolver before calling MakeReadOnly is not thread safe and can lead to InvalidOperationExceptions like those in dotnet/runtime#89830 when there's a race.

At first blush, GetReadOnlyTypeInfo and the SystemTextJsonOutputFormatter ctor appear to have the same thread safety issue. Of course, if we can guarantee these never run concurrently, there might not be an issue. That could explain why our tests haven't caught this, but I think it's more likely that we just don't have any functional tests where TypeInfoResolver would be null by the time it gets to this code.

aspnetcore/src/Shared/Json/JsonSerializerExtensions.cs

Lines 19 to 25 in f1cfa8e

	public static JsonTypeInfo GetReadOnlyTypeInfo(this JsonSerializerOptions options, Type type)
	{
	// Use JsonTypeInfoResolver.Combine() to produce an empty TypeInfoResolver
	options.TypeInfoResolver ??= JsonTypeInfoResolver.Combine();
	options.MakeReadOnly();
	return options.GetTypeInfo(type);
	}

aspnetcore/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs

Lines 26 to 28 in f1cfa8e

	// Use JsonTypeInfoResolver.Combine() to produce an empty TypeInfoResolver
	jsonSerializerOptions.TypeInfoResolver ??= JsonTypeInfoResolver.Combine();
	jsonSerializerOptions.MakeReadOnly();

It appears we won't be able to use the newly approved thread-safe MakeReadOnly(bool populateMissingResolver) API because we try to set an empty rather than the default reflection-based TypeInfoResolver. I'm not sure why that is. Is it an attempt to reduce the need for suppressions when we're doing serialization that is not statically verifiable to be linker safe?

I get that it's probably unusual to get a null TypeInfoResolver unless reflection is really not supported given who JsonOptions.DefaultSerializerOptions gets configured, but it still seems nicer to at least try to fallback to reflection rather than just fail to serialize or deserialize if there's even a small chance that it might work. Wouldn't it effectively just change the exception that's thrown if the reflection fallback fails? Probably to something with a clearer error message?

@eerhardt @eiriktsarpalis @captainsafia

[eiriktsarpalis]

It appears we won't be able to use the newly approved thread-safe MakeReadOnly(bool populateMissingResolver) API because we try to set an empty rather than the default reflection-based TypeInfoResolver.

To clarify, the new method will populate using the default resolver depending on the setting of JsonSerializer.IsReflectionEnabledByDefault. So reflection-based resolver if that's true, empty resolver if it's false. I suspect that might satisfy your use case (but you'd still need to add a suppression on the method call).

[halter73]

I think fixing this should only prevent a small race where we throw the wrong exception when reflection really is disabled, or something manually set JsonOptions.SerializerOptions.TypeInfoResolver to null for some reason. In the latter case, calling MakeReadOnly(true) might just make that work, but I doubt many apps would do that. I now think this is even lower impact than when I did when I started filing the issue, but I still think it's probably a bug.

[eerhardt]

because we try to set an empty rather than the default reflection-based TypeInfoResolver. I'm not sure why that is.

#49393

[halter73]

I saw that PR, but that doesn't explain why we don't make a last-ditch effort to try reflection anyway and just suppress the warnings. Of course, it probably won't work anyway, but it might, and it would be thread safe with the new API.

This would require only attempt using TypeInfoResolver at the last moment after we know we're actually going to need to use it. Based on my investigation so far on #49855, it doesn't appear that we're as lazy as we need to be. SystemTextJsonOutputFormatter would have to use the TypeInfoResolver more lazily too.

[eerhardt]

why we don't make a last-ditch effort to try reflection anyway and just suppress the warnings.

Suppressing warnings is bad, that is not our first choice (or even second choice) for making things work. The cases where options.TypeInfoResolver is null at this point are:

1. You are in a project where JsonSerializer.IsReflectionEnabledByDefault == false (i.e. PublishAot or PublishTrimmed) and you didn't configure a TypeInfoResolver in your app. This is explicitly where we don't want to try to use reflection. We've disabled JsonSerializer.IsReflectionEnabledByDefault, why would we then go and try to use reflection by default?
2. Someone explicitly set it to null. In which case... why would we then go and try to use reflection by default?

[eerhardt]

IMO - if we really care about this thread safety issue, then JsonSerializerOptions.MakeReadOnly() should allow for TypeInfoResolver == null. Then we would even need to set it in the first place. (cc @eiriktsarpalis).

Either that, or we shouldn't need to call MakeReadOnly().

[eerhardt]

but it still seems nicer to at least try to fallback to reflection rather than just fail to serialize or deserialize if there's even a small chance that it might work. Wouldn't it effectively just change the exception that's thrown if the reflection fallback fails?

To explain more here: We 100% don't want to try to fallback to reflection when PublishAot/Trimmed=true is set for the project. Our whole goal/experience is that your app works the same at F5 time as it does when you publish it. If we tried using reflection here, reflection would work just fine at F5 time (because we run on CoreCLR which can MakeGenericType and JIT at runtime). So your app works while debugging. Then you publish it and the app gets AOT'd which can't MakeGenericType and JIT at runtime. And your app breaks.

[eiriktsarpalis]

Someone explicitly set it to null. In which case... why would we then go and try to use reflection by default?

From STJ perspective there's no difference between unconfigured and explicitly setting to null. If IsReflectionEnabledByDefault is true then you'd want to default to reflection since that's what JsonSerializer is doing.

then JsonSerializerOptions.MakeReadOnly() should allow for TypeInfoResolver == null.

And fall back to the empty resolver that supports no types? That's almost always invalid configuration -- I would say it's preferable to fail fast so that the user can correct it. A sane fallback is what is offered by the new method added in dotnet/runtime#90013, but that must necessarily be marked RUC/RDC.

We 100% don't want to try to fallback to reflection when PublishAot/Trimmed=true is set for the project.

Agreed. The more I think about it, the more I think that the new options.MakeReadOnly(populateResolver: true); method is the solution that you need since it honors IsReflectionEnabledByDefault: if true it populates with reflection and if false it fails with InvalidOperationException. That does mean it will fail at configuration time, but I think that failing fast is preferable in this case: there is no valid scenario where you'd want your application to initialize successfully with an empty resolver (unless that's what the user explicitly wanted).