From 8b4eaf94140f488743d0c78caa3afec3b9c5d789 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 7 Mar 2022 19:44:14 +0000 Subject: [PATCH] Fix issues related to JsonSerializerOptions mutation and caching. (#66248) * Fix issues related to JsonSerializerOptions mutation and caching. (#65863) * Fix issues related to JsonSerializerOptions mutation and caching. * fix test style * fix linker warning * disable failing tests in netfx --- .../System.Text.Json/src/Resources/Strings.resx | 3 -- .../Converters/JsonMetadataServicesConverter.cs | 2 +- .../Json/Serialization/JsonSerializer.Helpers.cs | 4 +- .../Serialization/JsonSerializer.Read.Stream.cs | 4 +- .../Serialization/JsonSerializer.Write.Helpers.cs | 6 +-- .../Json/Serialization/JsonSerializerContext.cs | 21 +--------- .../Serialization/JsonSerializerOptions.Caching.cs | 11 +++++- .../JsonSerializerOptions.Converters.cs | 24 +++++++++-- .../Json/Serialization/JsonSerializerOptions.cs | 46 +++++++++++----------- .../Serialization/Metadata/JsonTypeInfo.Cache.cs | 4 +- .../System/Text/Json/ThrowHelper.Serialization.cs | 6 --- .../JsonSerializerContextTests.cs | 1 + .../Serialization/CacheTests.cs | 10 ++--- .../CustomConverterTests/CustomConverterTests.cs | 36 +++++++++++++++++ .../Serialization/OptionsTests.cs | 2 + 15 files changed, 109 insertions(+), 71 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index c8759d3..100a206 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -569,9 +569,6 @@ A 'JsonSerializerOptions' instance associated with a 'JsonSerializerContext' instance cannot be mutated once the context has been instantiated. - - "The specified 'JsonSerializerOptions' instance is already bound with a 'JsonSerializerContext' instance." - The generic type of the converter for property '{0}.{1}' must match with the specified converter type '{2}'. The converter must not be 'null'. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs index 599f57c..61ce1b8 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs @@ -76,7 +76,7 @@ namespace System.Text.Json.Serialization.Converters if (!state.SupportContinuation && jsonTypeInfo is JsonTypeInfo info && info.SerializeHandler != null && - info.Options._serializerContext?.CanUseSerializationLogic == true) + info.Options.JsonSerializerContext?.CanUseSerializationLogic == true) { info.SerializeHandler(writer, value); return true; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs index fa39f8d..7168618 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs @@ -18,9 +18,9 @@ namespace System.Text.Json Debug.Assert(runtimeType != null); options ??= JsonSerializerOptions.Default; - if (!JsonSerializerOptions.IsInitializedForReflectionSerializer) + if (!options.IsInitializedForReflectionSerializer) { - JsonSerializerOptions.InitializeForReflectionSerializer(); + options.InitializeForReflectionSerializer(); } return options.GetOrAddJsonTypeInfoForRootType(runtimeType); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs index 10ede4e..cd8584f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs @@ -287,9 +287,9 @@ namespace System.Text.Json CancellationToken cancellationToken = default) { options ??= JsonSerializerOptions.Default; - if (!JsonSerializerOptions.IsInitializedForReflectionSerializer) + if (!options.IsInitializedForReflectionSerializer) { - JsonSerializerOptions.InitializeForReflectionSerializer(); + options.InitializeForReflectionSerializer(); } return CreateAsyncEnumerableDeserializer(utf8Json, options, cancellationToken); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs index 45a39dc..607cb38 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs @@ -41,7 +41,7 @@ namespace System.Text.Json if (jsonTypeInfo.HasSerialize && jsonTypeInfo is JsonTypeInfo typedInfo && - typedInfo.Options._serializerContext?.CanUseSerializationLogic == true) + typedInfo.Options.JsonSerializerContext?.CanUseSerializationLogic == true) { Debug.Assert(typedInfo.SerializeHandler != null); typedInfo.SerializeHandler(writer, value); @@ -59,8 +59,8 @@ namespace System.Text.Json Debug.Assert(!jsonTypeInfo.HasSerialize || jsonTypeInfo is not JsonTypeInfo || - jsonTypeInfo.Options._serializerContext == null || - !jsonTypeInfo.Options._serializerContext.CanUseSerializationLogic, + jsonTypeInfo.Options.JsonSerializerContext == null || + !jsonTypeInfo.Options.JsonSerializerContext.CanUseSerializationLogic, "Incorrect method called. WriteUsingGeneratedSerializer() should have been called instead."); WriteStack state = default; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs index ca4c2bf..0b8385b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs @@ -21,19 +21,7 @@ namespace System.Text.Json.Serialization /// /// The instance cannot be mutated once it is bound with the context instance. /// - public JsonSerializerOptions Options - { - get - { - if (_options == null) - { - _options = new JsonSerializerOptions(); - _options._serializerContext = this; - } - - return _options; - } - } + public JsonSerializerOptions Options => _options ??= new JsonSerializerOptions { JsonSerializerContext = this }; /// /// Indicates whether pre-generated serialization logic for types in the context @@ -95,13 +83,8 @@ namespace System.Text.Json.Serialization { if (options != null) { - if (options._serializerContext != null) - { - ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext(); - } - + options.JsonSerializerContext = this; _options = options; - options._serializerContext = this; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs index 774bd81..75f7b98 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs @@ -74,6 +74,10 @@ namespace System.Text.Json private void InitializeCachingContext() { _cachingContext = TrackedCachingContexts.GetOrCreate(this); + if (IsInitializedForReflectionSerializer) + { + _cachingContext.Options.IsInitializedForReflectionSerializer = true; + } } /// @@ -159,7 +163,12 @@ namespace System.Text.Json // Use a defensive copy of the options instance as key to // avoid capturing references to any caching contexts. - var key = new JsonSerializerOptions(options) { _serializerContext = options._serializerContext }; + var key = new JsonSerializerOptions(options) + { + // Copy fields ignored by the copy constructor + // but are necessary to determine equivalence. + _serializerContext = options._serializerContext, + }; Debug.Assert(key._cachingContext == null); ctx = new CachingContext(options); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index 6d3a3c8..9126424 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -23,11 +23,27 @@ namespace System.Text.Json // The global list of built-in converters that override CanConvert(). private static JsonConverter[]? s_defaultFactoryConverters; + // Stores the JsonTypeInfo factory, which requires unreferenced code and must be rooted by the reflection-based serializer. + private static Func? s_typeInfoCreationFunc; + + [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] + private static void RootReflectionSerializerDependencies() + { + if (s_defaultSimpleConverters is null) + { + s_defaultSimpleConverters = GetDefaultSimpleConverters(); + s_defaultFactoryConverters = GetDefaultFactoryConverters(); + s_typeInfoCreationFunc = CreateJsonTypeInfo; + } + + [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] + static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options); + } + [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - private static void RootBuiltInConverters() + private static JsonConverter[] GetDefaultFactoryConverters() { - s_defaultSimpleConverters = GetDefaultSimpleConverters(); - s_defaultFactoryConverters = new JsonConverter[] + return new JsonConverter[] { // Check for disallowed types. new UnsupportedTypeConverterFactory(), @@ -159,7 +175,7 @@ namespace System.Text.Json [RequiresUnreferencedCode("Getting a converter for a type may require reflection which depends on unreferenced code.")] public JsonConverter GetConverter(Type typeToConvert!!) { - RootBuiltInConverters(); + RootReflectionSerializerDependencies(); return GetConverterInternal(typeToConvert); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs index caa8066..95199d7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs @@ -33,13 +33,8 @@ namespace System.Text.Json /// public static JsonSerializerOptions Default { get; } = CreateDefaultImmutableInstance(); - internal JsonSerializerContext? _serializerContext; - - // Stores the JsonTypeInfo factory, which requires unreferenced code and must be rooted by the reflection-based serializer. - private static Func? s_typeInfoCreationFunc; - // For any new option added, adding it to the options copied in the copy constructor below must be considered. - + private JsonSerializerContext? _serializerContext; private MemberAccessor? _memberAccessorStrategy; private JsonNamingPolicy? _dictionaryKeyPolicy; private JsonNamingPolicy? _jsonPropertyNamingPolicy; @@ -143,7 +138,7 @@ namespace System.Text.Json } /// - /// Binds current instance with a new instance of the specified type. + /// Binds current instance with a new instance of the specified type. /// /// The generic definition of the specified context type. /// When serializing and deserializing types using the options @@ -151,11 +146,7 @@ namespace System.Text.Json /// public void AddContext() where TContext : JsonSerializerContext, new() { - if (_serializerContext != null) - { - ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext(); - } - + VerifyMutable(); TContext context = new(); _serializerContext = context; context._options = this; @@ -550,6 +541,16 @@ namespace System.Text.Json } } + internal JsonSerializerContext? JsonSerializerContext + { + get => _serializerContext; + set + { + VerifyMutable(); + _serializerContext = value; + } + } + // The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycles semanitcs or None of them. internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None; @@ -576,27 +577,25 @@ namespace System.Text.Json } /// - /// Whether needs to be called. + /// Whether the options instance has been primed for reflection-based serialization. /// - internal static bool IsInitializedForReflectionSerializer { get; set; } + internal bool IsInitializedForReflectionSerializer { get; private set; } /// /// Initializes the converters for the reflection-based serializer. /// must be checked before calling. /// [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - internal static void InitializeForReflectionSerializer() + internal void InitializeForReflectionSerializer() { - // For threading cases, the state that is set here can be overwritten. - RootBuiltInConverters(); - s_typeInfoCreationFunc = CreateJsonTypeInfo; + RootReflectionSerializerDependencies(); IsInitializedForReflectionSerializer = true; - - [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options); + if (_cachingContext != null) + { + _cachingContext.Options.IsInitializedForReflectionSerializer = true; + } } - private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type) { JsonTypeInfo? info = _serializerContext?.GetTypeInfo(type); @@ -605,12 +604,13 @@ namespace System.Text.Json return info; } - if (s_typeInfoCreationFunc == null) + if (!IsInitializedForReflectionSerializer) { ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type); return null!; } + Debug.Assert(s_typeInfoCreationFunc != null); return s_typeInfoCreationFunc(type, this); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index e138cc7..50648a8 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -574,7 +574,7 @@ namespace System.Text.Json.Serialization.Metadata Debug.Assert(PropertyCache == null); Debug.Assert(PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Object); - JsonSerializerContext? context = Options._serializerContext; + JsonSerializerContext? context = Options.JsonSerializerContext; Debug.Assert(context != null); JsonPropertyInfo[] array; @@ -630,7 +630,7 @@ namespace System.Text.Json.Serialization.Metadata Debug.Assert(PropertyCache != null); Debug.Assert(PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Object); - JsonSerializerContext? context = Options._serializerContext; + JsonSerializerContext? context = Options.JsonSerializerContext; Debug.Assert(context != null); JsonParameterInfoValues[] array; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs index 8668660..6ff8cc5 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs @@ -579,12 +579,6 @@ namespace System.Text.Json } [DoesNotReturn] - public static void ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext() - { - throw new InvalidOperationException(SR.Format(SR.OptionsAlreadyBoundToContext)); - } - - [DoesNotReturn] public static void ThrowNotSupportedException_BuiltInConvertersNotRooted(Type type) { throw new NotSupportedException(SR.Format(SR.BuiltInConvertersNotRooted, type)); diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs index 6442dbb..bbba444 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs @@ -38,6 +38,7 @@ namespace System.Text.Json.SourceGeneration.Tests // This test uses reflection to: // - Access JsonSerializerOptions.s_defaultSimpleConverters // - Access JsonSerializerOptions.s_defaultFactoryConverters + // - Access JsonSerializerOptions.s_typeInfoCreationFunc // // If any of them changes, this test will need to be kept in sync. diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs index 8b4bd83..37daa9b 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs @@ -255,9 +255,9 @@ namespace System.Text.Json.Serialization.Tests } } + [ActiveIssue("https://github.com/dotnet/runtime/issues/66232", TargetFrameworkMonikers.NetFramework)] [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [MemberData(nameof(GetJsonSerializerOptions))] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] public static void JsonSerializerOptions_ReuseConverterCaches() { // This test uses reflection to: @@ -286,10 +286,12 @@ namespace System.Text.Json.Serialization.Tests for (int i = 0; i < 5; i++) { var options2 = new JsonSerializerOptions(options); - Assert.True(equalityComparer.Equals(options2, originalCacheOptions)); - Assert.Equal(equalityComparer.GetHashCode(options2), equalityComparer.GetHashCode(originalCacheOptions)); Assert.Null(getCacheOptions(options2)); + JsonSerializer.Serialize(42, options2); + + Assert.True(equalityComparer.Equals(options2, originalCacheOptions)); + Assert.Equal(equalityComparer.GetHashCode(options2), equalityComparer.GetHashCode(originalCacheOptions)); Assert.Same(originalCacheOptions, getCacheOptions(options2)); } } @@ -318,7 +320,6 @@ namespace System.Text.Json.Serialization.Tests } [Fact] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] public static void JsonSerializerOptions_EqualityComparer_ChangingAnySettingShouldReturnFalse() { // This test uses reflection to: @@ -386,7 +387,6 @@ namespace System.Text.Json.Serialization.Tests } [Fact] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] public static void JsonSerializerOptions_EqualityComparer_ApplyingJsonSerializerContextShouldReturnFalse() { // This test uses reflection to: diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs index 8006729..0ec72a2 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs @@ -1,6 +1,9 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; +using System.IO; +using Microsoft.DotNet.RemoteExecutor; using Xunit; namespace System.Text.Json.Serialization.Tests @@ -176,6 +179,39 @@ namespace System.Text.Json.Serialization.Tests } } + [ActiveIssue("https://github.com/dotnet/runtime/issues/66232", TargetFrameworkMonikers.NetFramework)] + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + public static void GetConverter_Poco_WriteThrowsNotSupportedException() + { + RemoteExecutor.Invoke(() => + { + JsonSerializerOptions options = new(); + JsonConverter converter = (JsonConverter)options.GetConverter(typeof(Point_2D)); + + using var writer = new Utf8JsonWriter(new MemoryStream()); + var value = new Point_2D(0, 0); + + // Running the converter without priming the options instance + // for reflection-based serialization should throw NotSupportedException + // since it can't resolve reflection-based metadata. + Assert.Throws(() => converter.Write(writer, value, options)); + Debug.Assert(writer.BytesCommitted + writer.BytesPending == 0); + + JsonSerializer.Serialize(42, options); + + // Same operation should succeed when instance has been primed. + converter.Write(writer, value, options); + Debug.Assert(writer.BytesCommitted + writer.BytesPending > 0); + writer.Reset(); + + // State change should not leak into unrelated options instances. + var options2 = new JsonSerializerOptions(); + options2.AddContext(); + Assert.Throws(() => converter.Write(writer, value, options2)); + Debug.Assert(writer.BytesCommitted + writer.BytesPending == 0); + }).Dispose(); + } + [Fact] public static void GetConverterTypeToConvertNull() { diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs index b034f23..791fe69 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs @@ -532,6 +532,8 @@ namespace System.Text.Json.Serialization.Tests var optionsSingleton = JsonSerializerOptions.Default; Assert.Throws(() => optionsSingleton.IncludeFields = true); Assert.Throws(() => optionsSingleton.Converters.Add(new JsonStringEnumConverter())); + Assert.Throws(() => optionsSingleton.AddContext()); + Assert.Throws(() => new JsonContext(optionsSingleton)); } [Fact] -- 2.7.4