MethodBase.GetParameters() is not thread-safe

[koenigst]

Description

If multiple threads call System.Reflection.MethodBase.GetParameters() at the same time, the resulting ParameterInfos are not always Equals even though they represent the same parameter.

Reproduction Steps

using System;
using System.Linq;
using System.Reflection;
using Xunit;

public sealed class ReflectionThreadSafety
{
    [Fact]
    public void SequentialBehavior()
    {
        var method = typeof(ReflectionThreadSafety).GetMethod("SequentialMethod", BindingFlags.Static | BindingFlags.NonPublic)!;
        var parameters = new[]
        {
            method.GetParameters()[0],
            method.GetParameters()[0],
        };
        Assert.All(parameters, p => Assert.Equal(parameters[0], p));
    }

    [Fact]
    public void ConcurrentBehavior()
    {
        var method = typeof(ReflectionThreadSafety).GetMethod("ConcurrentMethod", BindingFlags.Static | BindingFlags.NonPublic)!;
        var parameters = Enumerable.Range(0, 2 * Environment.ProcessorCount)
            .AsParallel()
            .WithExecutionMode(ParallelExecutionMode.ForceParallelism)
            .Select(_ => method.GetParameters()[0])
            .ToList();
        Assert.All(parameters, p => Assert.Equal(parameters[0], p));
    }

    private static void SequentialMethod(int p, DateTimeOffset o, TimeSpan t, AppDomain a) { }
    private static void ConcurrentMethod(int p, DateTimeOffset o, TimeSpan t, AppDomain a) { }
}

Expected behavior

Sequential and concurrent access should behave the same.

Actual behavior

The concurrent test sometimes fails (repeat test run a few times as the concurrency issue can not be reproduced reliably). The likely cause is that multiple threads fill the RuntimeMethodInfo.m_parameters cache resulting in multiple instances of ParameterInfo for the same method parameter. Most information on the instances are identical but because ParameterInfo.Equals is based on reference equality the test fails.

Regression?

The code is essentially the same as in .NET Framework 4.8.1 and the relevant .NET Core code has not changed in the last five years.

Known Workarounds

Call MethodBase.GetParameters from a single thread before accessing it in parallel. Once the cache is initialised all further concurrent calls will return the correct parameter instances.

Configuration

• TargetFramework: net8.0
• .NET SDK: 8.0.101
• Architecture: x64
• OS: Windows 11 Enterprise 22H2 (OS build 22621.3155)

Other information

Calling reflection code concurrently seems to be a rather unusual case because most of the reflection code uses the same not completely thread-safe caching mechanism and I have not run into this issue in the last ten years of .NET use. I am also aware that making the caching thread-safe has some overhead that is not desired for dealing with a rare occurence. The documentation declares the class as thread-safe. Would it make sense to change the documentation instead of the implementation?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Description If multiple threads call `System.Reflection.MethodBase.GetParameters()` at the same time, the resulting `ParameterInfo`s are not always `Equals` even though they represent the same parameter. ### Reproduction Steps ```csharp using System; using System.Linq; using System.Reflection; using Xunit; public sealed class ReflectionThreadSafety { [Fact] public void SequentialBehavior() { var method = typeof(ReflectionThreadSafety).GetMethod("SequentialMethod", BindingFlags.Static | BindingFlags.NonPublic)!; var parameters = new[] { method.GetParameters()[0], method.GetParameters()[0], }; Assert.All(parameters, p => Assert.Equal(parameters[0], p)); } [Fact] public void ConcurrentBehavior() { var method = typeof(ReflectionThreadSafety).GetMethod("ConcurrentMethod", BindingFlags.Static | BindingFlags.NonPublic)!; var parameters = Enumerable.Range(0, 2 * Environment.ProcessorCount) .AsParallel() .WithExecutionMode(ParallelExecutionMode.ForceParallelism) .Select(_ => method.GetParameters()[0]) .ToList(); Assert.All(parameters, p => Assert.Equal(parameters[0], p)); } private static void SequentialMethod(int p, DateTimeOffset o, TimeSpan t, AppDomain a) { } private static void ConcurrentMethod(int p, DateTimeOffset o, TimeSpan t, AppDomain a) { } } ``` ### Expected behavior Sequential and concurrent access should behave the same. ### Actual behavior The concurrent test sometimes fails (repeat test run a few times as the concurrency issue can not be reproduced reliably). The likely cause is that multiple threads fill the [`RuntimeMethodInfo.m_parameters`](https://github.com/dotnet/runtime/blob/release/9.0-preview2/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs#L23) cache resulting in multiple instances of `ParameterInfo` for the same method parameter. Most information on the instances are identical but because `ParameterInfo.Equals` is based on reference equality the test fails. ### Regression? The code is essentially the same as in .NET Framework 4.8.1 and the relevant .NET Core code has not changed in the last five years. ### Known Workarounds Call `MethodBase.GetParameters` from a single thread before accessing it in parallel. Once the cache is initialised all further concurrent calls will return the correct parameter instances. ### Configuration * TargetFramework: net8.0 * .NET SDK: 8.0.101 * Architecture: x64 * OS: Windows 11 Enterprise 22H2 (OS build 22621.3155) ### Other information Calling reflection code concurrently seems to be a rather unusual case because most of the reflection code uses the same not completely thread-safe caching mechanism and I have not run into this issue in the last ten years of .NET use. I am also aware that making the caching thread-safe has some overhead that is not desired for dealing with a rare occurence. The [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.reflection.methodbase#thread-safety) declares the class as thread-safe. Would it make sense to change the documentation instead of the implementation?

Author:	koenigst
Assignees:	-
Labels:	`area-System.Reflection`
Milestone:	-

[steveharter]

I assume Equal() is equivalent to ReferenceEquals() in this case.

[koenigst]

As far as I can see yes. ParameterInfo and RuntimeParameterInfo inherit their equality implementation from object. I think this is frequently the case with System.Reflection classes.