NullabilityInfoContext is not thread-safe

[jozkee]

Description

Found while looking into #100144.

When caching the NullabilityInfoContext to use it in the reflection-based System.Text.Json serializer, I noticed that the context is not thread-safe, It would be nice that the type would implement a thread-safety mechanism like a Concurrent Dictionary for the MemberInfos that it caches.

Reproduction Steps

Here's a simple repro that shows the failure by just trying to parallelize NullabilityInfo creation of the properties in a type.

using System.Reflection;
using System.Text.Json;

NullabilityInfoContext ctx = new NullabilityInfoContext();
PropertyInfo[] properties = typeof(JsonSerializerOptions).GetProperties(BindingFlags.Public | BindingFlags.Instance);

Parallel.ForEach(properties, property =>
{
    NullabilityInfo nullabilityInfo = ctx.Create(property);
    Console.WriteLine($"Property: {property.Name}, Getter nullability: {nullabilityInfo.ReadState}");
});

Expected behavior

Not to throw on parallelized scenarios.

Actual behavior

Property: IgnoreReadOnlyProperties, Getter nullability: NotNull
Property: Converters, Getter nullability: NotNull
Property: AllowOutOfOrderMetadataProperties, Getter nullability: NotNull
Property: Encoder, Getter nullability: Nullable
Property: WriteIndented, Getter nullability: NotNull
Property: DefaultIgnoreCondition, Getter nullability: NotNull
Property: IndentCharacter, Getter nullability: NotNull
Property: ReferenceHandler, Getter nullability: Nullable
Property: PropertyNamingPolicy, Getter nullability: Nullable
Property: DictionaryKeyPolicy, Getter nullability: Nullable
Property: UnknownTypeHandling, Getter nullability: NotNull
Property: NumberHandling, Getter nullability: NotNull
Property: IgnoreReadOnlyFields, Getter nullability: NotNull
Property: PreferredObjectCreationHandling, Getter nullability: NotNull
Property: DefaultBufferSize, Getter nullability: NotNull
Property: PropertyNameCaseInsensitive, Getter nullability: NotNull
Property: IncludeFields, Getter nullability: NotNull
Property: IgnoreNullValues, Getter nullability: NotNull
Property: IndentSize, Getter nullability: NotNull
Property: TypeInfoResolver, Getter nullability: Nullable
Property: UnmappedMemberHandling, Getter nullability: NotNull
Property: AllowTrailingCommas, Getter nullability: NotNull
Property: MaxDepth, Getter nullability: NotNull
Property: ReadCommentHandling, Getter nullability: NotNull
Unhandled exception. System.AggregateException: One or more errors occurred. (Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.)
 ---> System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Reflection.NullabilityInfoContext.GetNullableContext(MemberInfo memberInfo)
   at System.Reflection.NullabilityInfoContext.GetNullabilityInfo(MemberInfo memberInfo, Type type, NullableAttributeStateParser parser, Int32& index)
   at System.Reflection.NullabilityInfoContext.Create(PropertyInfo propertyInfo)
   at nullability_ctx_info.Program.<>c__DisplayClass0_0.<Main>b__0(PropertyInfo property) in C:\Users\dacantu\consoleapps\nullability-ctx-info\Program.cs:line 19
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
   at System.Threading.Tasks.TaskReplicator.Replica.Execute()
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.TaskReplicator.Run[TState](ReplicatableUserAction`1 action, ParallelOptions options, Boolean stopOnFirstFailure)
   at System.Threading.Tasks.Parallel.ForWorker[TLocal,TInt](TInt fromInclusive, TInt toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Parallel.ForWorker[TLocal,TInt](TInt fromInclusive, TInt toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
   at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
   at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, Action`1 body)
   at nullability_ctx_info.Program.Main(String[] args) in C:\Users\dacantu\consoleapps\nullability-ctx-info\Program.cs:line 17

Regression?

No

Known Workarounds

Create a new NullabilityInfoContext() inside Parallel.ForEach but this will slow-down the execution.

Configuration

No response

Other information

No response

[jozkee]

cc @eiriktsarpalis

[eiriktsarpalis]

I think that's by design, ideally you should try to create an ephemeral NullabilityInfoContext for the particular thread that is trying to resolve nullability annotations for a given type and have it collected when done.

cc @buyaa-n

[buyaa-n]

Yes, it was by design, it is mentioned in the approved API #29723 (comment)

We discussed the caching strategy, and since NullabilityInfo objects are expected to be freshly returned each time there's no parallel-caller mutation concern.

[jozkee]

You folks mean that the correct way to use this API in this case is as follows:

Parallel.ForEach(properties, property =>
{
    NullabilityInfo nullabilityInfo = new NullabilityInfoContext().Create(property);
    Console.WriteLine($"Property: {property.Name}, Getter nullability: {nullabilityInfo.ReadState}");
});

?

[eiriktsarpalis]

Yes, or ideally reuse the same context for all members in a type/type graph.