Skip to content

Commit 3ede809

Browse files
jkotasjkotas
authored
Revert "Fix issues related to JsonSerializerOptions mutation and caching. (#65863)" (#66235)
This reverts commit 11b7961.
1 parent 17662fc commit 3ede809

15 files changed

+71
-107
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,9 @@
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>
572575
<data name="ConverterForPropertyMustBeValid" xml:space="preserve">
573576
<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>
574577
</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.JsonSerializerContext?.CanUseSerializationLogic == true)
79+
info.Options._serializerContext?.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 (!options.IsInitializedForReflectionSerializer)
21+
if (!JsonSerializerOptions.IsInitializedForReflectionSerializer)
2222
{
23-
options.InitializeForReflectionSerializer();
23+
JsonSerializerOptions.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 (!options.IsInitializedForReflectionSerializer)
290+
if (!JsonSerializerOptions.IsInitializedForReflectionSerializer)
291291
{
292-
options.InitializeForReflectionSerializer();
292+
JsonSerializerOptions.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.JsonSerializerContext?.CanUseSerializationLogic == true)
44+
typedInfo.Options._serializerContext?.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.JsonSerializerContext == null ||
63-
!jsonTypeInfo.Options.JsonSerializerContext.CanUseSerializationLogic,
62+
jsonTypeInfo.Options._serializerContext == null ||
63+
!jsonTypeInfo.Options._serializerContext.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: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,19 @@ 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 => _options ??= new JsonSerializerOptions { JsonSerializerContext = this };
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+
}
2537

2638
/// <summary>
2739
/// Indicates whether pre-generated serialization logic for types in the context
@@ -83,8 +95,13 @@ protected JsonSerializerContext(JsonSerializerOptions? options)
8395
{
8496
if (options != null)
8597
{
86-
options.JsonSerializerContext = this;
98+
if (options._serializerContext != null)
99+
{
100+
ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext();
101+
}
102+
87103
_options = options;
104+
options._serializerContext = this;
88105
}
89106
}
90107

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

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

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

164160
// Use a defensive copy of the options instance as key to
165161
// avoid capturing references to any caching contexts.
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-
};
162+
var key = new JsonSerializerOptions(options) { _serializerContext = options._serializerContext };
172163
Debug.Assert(key._cachingContext == null);
173164

174165
ctx = new CachingContext(options);

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

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,11 @@ 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-
4326
[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
44-
private static JsonConverter[] GetDefaultFactoryConverters()
27+
private static void RootBuiltInConverters()
4528
{
46-
return new JsonConverter[]
29+
s_defaultSimpleConverters = GetDefaultSimpleConverters();
30+
s_defaultFactoryConverters = new JsonConverter[]
4731
{
4832
// Check for disallowed types.
4933
new UnsupportedTypeConverterFactory(),
@@ -175,7 +159,7 @@ internal JsonConverter GetConverterFromMember(Type? parentClassType, Type proper
175159
[RequiresUnreferencedCode("Getting a converter for a type may require reflection which depends on unreferenced code.")]
176160
public JsonConverter GetConverter(Type typeToConvert!!)
177161
{
178-
RootReflectionSerializerDependencies();
162+
RootBuiltInConverters();
179163
return GetConverterInternal(typeToConvert);
180164
}
181165

‎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,8 +33,13 @@ 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+
3641
// For any new option added, adding it to the options copied in the copy constructor below must be considered.
37-
private JsonSerializerContext? _serializerContext;
42+
3843
private MemberAccessor? _memberAccessorStrategy;
3944
private JsonNamingPolicy? _dictionaryKeyPolicy;
4045
private JsonNamingPolicy? _jsonPropertyNamingPolicy;
@@ -138,15 +143,19 @@ public JsonSerializerOptions(JsonSerializerDefaults defaults) : this()
138143
}
139144

140145
/// <summary>
141-
/// Binds current <see cref="JsonSerializerOptions"/> instance with a new instance of the specified <see cref="Serialization.JsonSerializerContext"/> type.
146+
/// Binds current <see cref="JsonSerializerOptions"/> instance with a new instance of the specified <see cref="JsonSerializerContext"/> type.
142147
/// </summary>
143148
/// <typeparam name="TContext">The generic definition of the specified context type.</typeparam>
144149
/// <remarks>When serializing and deserializing types using the options
145150
/// instance, metadata for the types will be fetched from the context instance.
146151
/// </remarks>
147152
public void AddContext<TContext>() where TContext : JsonSerializerContext, new()
148153
{
149-
VerifyMutable();
154+
if (_serializerContext != null)
155+
{
156+
ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext();
157+
}
158+
150159
TContext context = new();
151160
_serializerContext = context;
152161
context._options = this;
@@ -541,16 +550,6 @@ public ReferenceHandler? ReferenceHandler
541550
}
542551
}
543552

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

@@ -577,25 +576,27 @@ internal MemberAccessor MemberAccessorStrategy
577576
}
578577

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

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

599+
599600
private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
600601
{
601602
JsonTypeInfo? info = _serializerContext?.GetTypeInfo(type);
@@ -604,13 +605,12 @@ private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
604605
return info;
605606
}
606607

607-
if (!IsInitializedForReflectionSerializer)
608+
if (s_typeInfoCreationFunc == null)
608609
{
609610
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type);
610611
return null!;
611612
}
612613

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.JsonSerializerContext;
577+
JsonSerializerContext? context = Options._serializerContext;
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.JsonSerializerContext;
633+
JsonSerializerContext? context = Options._serializerContext;
634634
Debug.Assert(context != null);
635635

636636
JsonParameterInfoValues[] array;

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,12 @@ internal static void ThrowUnexpectedMetadataException(
578578
}
579579
}
580580

581+
[DoesNotReturn]
582+
public static void ThrowInvalidOperationException_JsonSerializerOptionsAlreadyBoundToContext()
583+
{
584+
throw new InvalidOperationException(SR.Format(SR.OptionsAlreadyBoundToContext));
585+
}
586+
581587
[DoesNotReturn]
582588
public static void ThrowNotSupportedException_BuiltInConvertersNotRooted(Type type)
583589
{

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ 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
4241
//
4342
// If any of them changes, this test will need to be kept in sync.
4443

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

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

258258
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
259259
[MemberData(nameof(GetJsonSerializerOptions))]
260+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
260261
public static void JsonSerializerOptions_ReuseConverterCaches()
261262
{
262263
// This test uses reflection to:
@@ -285,12 +286,10 @@ public static void JsonSerializerOptions_ReuseConverterCaches()
285286
for (int i = 0; i < 5; i++)
286287
{
287288
var options2 = new JsonSerializerOptions(options);
288-
Assert.Null(getCacheOptions(options2));
289-
290-
JsonSerializer.Serialize(42, options2);
291-
292289
Assert.True(equalityComparer.Equals(options2, originalCacheOptions));
293290
Assert.Equal(equalityComparer.GetHashCode(options2), equalityComparer.GetHashCode(originalCacheOptions));
291+
Assert.Null(getCacheOptions(options2));
292+
JsonSerializer.Serialize(42, options2);
294293
Assert.Same(originalCacheOptions, getCacheOptions(options2));
295294
}
296295
}
@@ -319,6 +318,7 @@ public static IEnumerable<object[]> GetJsonSerializerOptions()
319318
}
320319

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

388388
[Fact]
389+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
389390
public static void JsonSerializerOptions_EqualityComparer_ApplyingJsonSerializerContextShouldReturnFalse()
390391
{
391392
// This test uses reflection to:

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

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
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;
74
using Xunit;
85

96
namespace System.Text.Json.Serialization.Tests
@@ -179,38 +176,6 @@ void RunTest<TConverterReturn>()
179176
}
180177
}
181178

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-
214179
[Fact]
215180
public static void GetConverterTypeToConvertNull()
216181
{

0 commit comments

Comments
 (0)