Skip to content

Commit 11b7961

Browse files
eiriktsarpaliseiriktsarpalis
authored
Fix issues related to JsonSerializerOptions mutation and caching. (#65863)
* Fix issues related to JsonSerializerOptions mutation and caching. * fix test style * fix linker warning
1 parent db73362 commit 11b7961

File tree

15 files changed

+107
-71
lines changed

15 files changed

+107
-71
lines changed

‎src/libraries/System.Text.Json/src/Resources/Strings.resx

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -569,9 +569,6 @@
569569
<data name="SerializerContextOptionsImmutable" xml:space="preserve">
570570
<value>A 'JsonSerializerOptions' instance associated with a 'JsonSerializerContext' instance cannot be mutated once the context has been instantiated.</value>
571571
</data>
572-
<data name="OptionsAlreadyBoundToContext" xml:space="preserve">
573-
<value>"The specified 'JsonSerializerOptions' instance is already bound with a 'JsonSerializerContext' instance."</value>
574-
</data>
575572
<data name="ConverterForPropertyMustBeValid" xml:space="preserve">
576573
<value>The generic type of the converter for property '{0}.{1}' must match with the specified converter type '{2}'. The converter must not be 'null'.</value>
577574
</data>

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ internal override bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializer
7676
if (!state.SupportContinuation &&
7777
jsonTypeInfo is JsonTypeInfo<T> info &&
7878
info.SerializeHandler != null &&
79-
info.Options._serializerContext?.CanUseSerializationLogic == true)
79+
info.Options.JsonSerializerContext?.CanUseSerializationLogic == true)
8080
{
8181
info.SerializeHandler(writer, value);
8282
return true;

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@ private static JsonTypeInfo GetTypeInfo(JsonSerializerOptions? options, Type run
1818
Debug.Assert(runtimeType != null);
1919

2020
options ??= JsonSerializerOptions.Default;
21-
if (!JsonSerializerOptions.IsInitializedForReflectionSerializer)
21+
if (!options.IsInitializedForReflectionSerializer)
2222
{
23-
JsonSerializerOptions.InitializeForReflectionSerializer();
23+
options.InitializeForReflectionSerializer();
2424
}
2525

2626
return options.GetOrAddJsonTypeInfoForRootType(runtimeType);

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,9 +287,9 @@ public static partial class JsonSerializer
287287
CancellationToken cancellationToken = default)
288288
{
289289
options ??= JsonSerializerOptions.Default;
290-
if (!JsonSerializerOptions.IsInitializedForReflectionSerializer)
290+
if (!options.IsInitializedForReflectionSerializer)
291291
{
292-
JsonSerializerOptions.InitializeForReflectionSerializer();
292+
options.InitializeForReflectionSerializer();
293293
}
294294

295295
return CreateAsyncEnumerableDeserializer(utf8Json, options, cancellationToken);

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ private static void WriteUsingGeneratedSerializer<TValue>(Utf8JsonWriter writer,
4141

4242
if (jsonTypeInfo.HasSerialize &&
4343
jsonTypeInfo is JsonTypeInfo<TValue> typedInfo &&
44-
typedInfo.Options._serializerContext?.CanUseSerializationLogic == true)
44+
typedInfo.Options.JsonSerializerContext?.CanUseSerializationLogic == true)
4545
{
4646
Debug.Assert(typedInfo.SerializeHandler != null);
4747
typedInfo.SerializeHandler(writer, value);
@@ -59,8 +59,8 @@ private static void WriteUsingSerializer<TValue>(Utf8JsonWriter writer, in TValu
5959

6060
Debug.Assert(!jsonTypeInfo.HasSerialize ||
6161
jsonTypeInfo is not JsonTypeInfo<TValue> ||
62-
jsonTypeInfo.Options._serializerContext == null ||
63-
!jsonTypeInfo.Options._serializerContext.CanUseSerializationLogic,
62+
jsonTypeInfo.Options.JsonSerializerContext == null ||
63+
!jsonTypeInfo.Options.JsonSerializerContext.CanUseSerializationLogic,
6464
"Incorrect method called. WriteUsingGeneratedSerializer() should have been called instead.");
6565

6666
WriteStack state = default;

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,7 @@ public abstract partial class JsonSerializerContext
2121
/// <remarks>
2222
/// The instance cannot be mutated once it is bound with the context instance.
2323
/// </remarks>
24-
public JsonSerializerOptions Options
25-
{
26-
get
27-
{
28-
if (_options == null)
29-
{
30-
_options = new JsonSerializerOptions();
31-
_options._serializerContext = this;
32-
}
33-
34-
return _options;
35-
}
36-
}
24+
public JsonSerializerOptions Options => _options ??= new JsonSerializerOptions { JsonSerializerContext = this };
3725

3826
/// <summary>
3927
/// Indicates whether pre-generated serialization logic for types in the context
@@ -95,13 +83,8 @@ protected JsonSerializerContext(JsonSerializerOptions? options)
9583
{
9684
if (options != null)
9785
{
98-
if (options._serializerContext != null)
99-
{
100-
ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext();
101-
}
102-
86+
options.JsonSerializerContext = this;
10387
_options = options;
104-
options._serializerContext = this;
10588
}
10689
}
10790

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ internal void ClearCaches()
7474
private void InitializeCachingContext()
7575
{
7676
_cachingContext = TrackedCachingContexts.GetOrCreate(this);
77+
if (IsInitializedForReflectionSerializer)
78+
{
79+
_cachingContext.Options.IsInitializedForReflectionSerializer = true;
80+
}
7781
}
7882

7983
/// <summary>
@@ -159,7 +163,12 @@ public static CachingContext GetOrCreate(JsonSerializerOptions options)
159163

160164
// Use a defensive copy of the options instance as key to
161165
// avoid capturing references to any caching contexts.
162-
var key = new JsonSerializerOptions(options) { _serializerContext = options._serializerContext };
166+
var key = new JsonSerializerOptions(options)
167+
{
168+
// Copy fields ignored by the copy constructor
169+
// but are necessary to determine equivalence.
170+
_serializerContext = options._serializerContext,
171+
};
163172
Debug.Assert(key._cachingContext == null);
164173

165174
ctx = new CachingContext(options);

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,27 @@ public sealed partial class JsonSerializerOptions
2323
// The global list of built-in converters that override CanConvert().
2424
private static JsonConverter[]? s_defaultFactoryConverters;
2525

26+
// Stores the JsonTypeInfo factory, which requires unreferenced code and must be rooted by the reflection-based serializer.
27+
private static Func<Type, JsonSerializerOptions, JsonTypeInfo>? s_typeInfoCreationFunc;
28+
29+
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
30+
private static void RootReflectionSerializerDependencies()
31+
{
32+
if (s_defaultSimpleConverters is null)
33+
{
34+
s_defaultSimpleConverters = GetDefaultSimpleConverters();
35+
s_defaultFactoryConverters = GetDefaultFactoryConverters();
36+
s_typeInfoCreationFunc = CreateJsonTypeInfo;
37+
}
38+
39+
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
40+
static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options);
41+
}
42+
2643
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
27-
private static void RootBuiltInConverters()
44+
private static JsonConverter[] GetDefaultFactoryConverters()
2845
{
29-
s_defaultSimpleConverters = GetDefaultSimpleConverters();
30-
s_defaultFactoryConverters = new JsonConverter[]
46+
return new JsonConverter[]
3147
{
3248
// Check for disallowed types.
3349
new UnsupportedTypeConverterFactory(),
@@ -159,7 +175,7 @@ internal JsonConverter GetConverterFromMember(Type? parentClassType, Type proper
159175
[RequiresUnreferencedCode("Getting a converter for a type may require reflection which depends on unreferenced code.")]
160176
public JsonConverter GetConverter(Type typeToConvert!!)
161177
{
162-
RootBuiltInConverters();
178+
RootReflectionSerializerDependencies();
163179
return GetConverterInternal(typeToConvert);
164180
}
165181

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,8 @@ public sealed partial class JsonSerializerOptions
3333
/// </remarks>
3434
public static JsonSerializerOptions Default { get; } = CreateDefaultImmutableInstance();
3535

36-
internal JsonSerializerContext? _serializerContext;
37-
38-
// Stores the JsonTypeInfo factory, which requires unreferenced code and must be rooted by the reflection-based serializer.
39-
private static Func<Type, JsonSerializerOptions, JsonTypeInfo>? s_typeInfoCreationFunc;
40-
4136
// For any new option added, adding it to the options copied in the copy constructor below must be considered.
42-
37+
private JsonSerializerContext? _serializerContext;
4338
private MemberAccessor? _memberAccessorStrategy;
4439
private JsonNamingPolicy? _dictionaryKeyPolicy;
4540
private JsonNamingPolicy? _jsonPropertyNamingPolicy;
@@ -143,19 +138,15 @@ public JsonSerializerOptions(JsonSerializerDefaults defaults) : this()
143138
}
144139

145140
/// <summary>
146-
/// Binds current <see cref="JsonSerializerOptions"/> instance with a new instance of the specified <see cref="JsonSerializerContext"/> type.
141+
/// Binds current <see cref="JsonSerializerOptions"/> instance with a new instance of the specified <see cref="Serialization.JsonSerializerContext"/> type.
147142
/// </summary>
148143
/// <typeparam name="TContext">The generic definition of the specified context type.</typeparam>
149144
/// <remarks>When serializing and deserializing types using the options
150145
/// instance, metadata for the types will be fetched from the context instance.
151146
/// </remarks>
152147
public void AddContext<TContext>() where TContext : JsonSerializerContext, new()
153148
{
154-
if (_serializerContext != null)
155-
{
156-
ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext();
157-
}
158-
149+
VerifyMutable();
159150
TContext context = new();
160151
_serializerContext = context;
161152
context._options = this;
@@ -550,6 +541,16 @@ public ReferenceHandler? ReferenceHandler
550541
}
551542
}
552543

544+
internal JsonSerializerContext? JsonSerializerContext
545+
{
546+
get => _serializerContext;
547+
set
548+
{
549+
VerifyMutable();
550+
_serializerContext = value;
551+
}
552+
}
553+
553554
// The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycles semanitcs or None of them.
554555
internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None;
555556

@@ -576,27 +577,25 @@ internal MemberAccessor MemberAccessorStrategy
576577
}
577578

578579
/// <summary>
579-
/// Whether <see cref="InitializeForReflectionSerializer()"/> needs to be called.
580+
/// Whether the options instance has been primed for reflection-based serialization.
580581
/// </summary>
581-
internal static bool IsInitializedForReflectionSerializer { get; set; }
582+
internal bool IsInitializedForReflectionSerializer { get; private set; }
582583

583584
/// <summary>
584585
/// Initializes the converters for the reflection-based serializer.
585586
/// <seealso cref="InitializeForReflectionSerializer"/> must be checked before calling.
586587
/// </summary>
587588
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
588-
internal static void InitializeForReflectionSerializer()
589+
internal void InitializeForReflectionSerializer()
589590
{
590-
// For threading cases, the state that is set here can be overwritten.
591-
RootBuiltInConverters();
592-
s_typeInfoCreationFunc = CreateJsonTypeInfo;
591+
RootReflectionSerializerDependencies();
593592
IsInitializedForReflectionSerializer = true;
594-
595-
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
596-
static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options);
593+
if (_cachingContext != null)
594+
{
595+
_cachingContext.Options.IsInitializedForReflectionSerializer = true;
596+
}
597597
}
598598

599-
600599
private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
601600
{
602601
JsonTypeInfo? info = _serializerContext?.GetTypeInfo(type);
@@ -605,12 +604,13 @@ private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
605604
return info;
606605
}
607606

608-
if (s_typeInfoCreationFunc == null)
607+
if (!IsInitializedForReflectionSerializer)
609608
{
610609
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type);
611610
return null!;
612611
}
613612

613+
Debug.Assert(s_typeInfoCreationFunc != null);
614614
return s_typeInfoCreationFunc(type, this);
615615
}
616616

‎src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,7 @@ internal void InitializePropCache()
574574
Debug.Assert(PropertyCache == null);
575575
Debug.Assert(PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Object);
576576

577-
JsonSerializerContext? context = Options._serializerContext;
577+
JsonSerializerContext? context = Options.JsonSerializerContext;
578578
Debug.Assert(context != null);
579579

580580
JsonPropertyInfo[] array;
@@ -630,7 +630,7 @@ internal void InitializeParameterCache()
630630
Debug.Assert(PropertyCache != null);
631631
Debug.Assert(PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Object);
632632

633-
JsonSerializerContext? context = Options._serializerContext;
633+
JsonSerializerContext? context = Options.JsonSerializerContext;
634634
Debug.Assert(context != null);
635635

636636
JsonParameterInfoValues[] array;

‎src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -591,12 +591,6 @@ internal static void ThrowUnexpectedMetadataException(
591591
}
592592
}
593593

594-
[DoesNotReturn]
595-
public static void ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext()
596-
{
597-
throw new InvalidOperationException(SR.Format(SR.OptionsAlreadyBoundToContext));
598-
}
599-
600594
[DoesNotReturn]
601595
public static void ThrowNotSupportedException_BuiltInConvertersNotRooted(Type type)
602596
{

‎src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ public static void Converters_AndTypeInfoCreator_NotRooted_WhenMetadataNotPresen
3838
// This test uses reflection to:
3939
// - Access JsonSerializerOptions.s_defaultSimpleConverters
4040
// - Access JsonSerializerOptions.s_defaultFactoryConverters
41+
// - Access JsonSerializerOptions.s_typeInfoCreationFunc
4142
//
4243
// If any of them changes, this test will need to be kept in sync.
4344

‎src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,6 @@ static Func<JsonSerializerOptions, int> CreateCacheCountAccessor()
257257

258258
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
259259
[MemberData(nameof(GetJsonSerializerOptions))]
260-
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
261260
public static void JsonSerializerOptions_ReuseConverterCaches()
262261
{
263262
// This test uses reflection to:
@@ -286,10 +285,12 @@ public static void JsonSerializerOptions_ReuseConverterCaches()
286285
for (int i = 0; i < 5; i++)
287286
{
288287
var options2 = new JsonSerializerOptions(options);
289-
Assert.True(equalityComparer.Equals(options2, originalCacheOptions));
290-
Assert.Equal(equalityComparer.GetHashCode(options2), equalityComparer.GetHashCode(originalCacheOptions));
291288
Assert.Null(getCacheOptions(options2));
289+
292290
JsonSerializer.Serialize(42, options2);
291+
292+
Assert.True(equalityComparer.Equals(options2, originalCacheOptions));
293+
Assert.Equal(equalityComparer.GetHashCode(options2), equalityComparer.GetHashCode(originalCacheOptions));
293294
Assert.Same(originalCacheOptions, getCacheOptions(options2));
294295
}
295296
}
@@ -318,7 +319,6 @@ public static IEnumerable<object[]> GetJsonSerializerOptions()
318319
}
319320

320321
[Fact]
321-
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
322322
public static void JsonSerializerOptions_EqualityComparer_ChangingAnySettingShouldReturnFalse()
323323
{
324324
// This test uses reflection to:
@@ -386,7 +386,6 @@ static IEnumerable<PropertyInfo> GetAllPublicPropertiesWithSetters()
386386
}
387387

388388
[Fact]
389-
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
390389
public static void JsonSerializerOptions_EqualityComparer_ApplyingJsonSerializerContextShouldReturnFalse()
391390
{
392391
// This test uses reflection to:

‎src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Diagnostics;
5+
using System.IO;
6+
using Microsoft.DotNet.RemoteExecutor;
47
using Xunit;
58

69
namespace System.Text.Json.Serialization.Tests
@@ -176,6 +179,38 @@ void RunTest<TConverterReturn>()
176179
}
177180
}
178181

182+
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
183+
public static void GetConverter_Poco_WriteThrowsNotSupportedException()
184+
{
185+
RemoteExecutor.Invoke(() =>
186+
{
187+
JsonSerializerOptions options = new();
188+
JsonConverter<Point_2D> converter = (JsonConverter<Point_2D>)options.GetConverter(typeof(Point_2D));
189+
190+
using var writer = new Utf8JsonWriter(new MemoryStream());
191+
var value = new Point_2D(0, 0);
192+
193+
// Running the converter without priming the options instance
194+
// for reflection-based serialization should throw NotSupportedException
195+
// since it can't resolve reflection-based metadata.
196+
Assert.Throws<NotSupportedException>(() => converter.Write(writer, value, options));
197+
Debug.Assert(writer.BytesCommitted + writer.BytesPending == 0);
198+
199+
JsonSerializer.Serialize(42, options);
200+
201+
// Same operation should succeed when instance has been primed.
202+
converter.Write(writer, value, options);
203+
Debug.Assert(writer.BytesCommitted + writer.BytesPending > 0);
204+
writer.Reset();
205+
206+
// State change should not leak into unrelated options instances.
207+
var options2 = new JsonSerializerOptions();
208+
options2.AddContext<JsonContext>();
209+
Assert.Throws<NotSupportedException>(() => converter.Write(writer, value, options2));
210+
Debug.Assert(writer.BytesCommitted + writer.BytesPending == 0);
211+
}).Dispose();
212+
}
213+
179214
[Fact]
180215
public static void GetConverterTypeToConvertNull()
181216
{

0 commit comments

Comments
 (0)