Fix broken private reflection in ArrayPool stress test

[ApparentlyPlus]

Fixes #128431

ThreadLocalIsCollectedUnderHighPressure looks up the internal GetMemoryPressure helper by reflection so it can spin until the pool reports high memory pressure. The lookup targets the wrong type:

var pressureMethod = ArrayPool<byte>.Shared.GetType()
    .GetMethod("GetMemoryPressure", BindingFlags.Static | BindingFlags.NonPublic);

ArrayPool<byte>.Shared is a SharedArrayPool<byte>, but GetMemoryPressure is a static on the separate internal System.Buffers.Utilities class. GetMethod therefore returns null, and the test throws NullReferenceException on the first Invoke, before it applies any memory pressure at all.

Why this went unnoticed

The test is a ConditionalFact gated on IsStressModeEnabledAndRemoteExecutorSupported, which requires DOTNET_TEST_STRESS=1. Normal CI never sets that, so the test is always skipped and the failure has been invisible since .NET 6.

The fix

Look the method up on System.Buffers.Utilities, and assert that both the type and the method resolve. The assertions are as much the point of the change as the corrected lookup, because a silent null is exactly what let this rot undetected for five years, so a future move of the helper now fails with a clear message instead of a bare NullReferenceException.

The #pragma warning disable IL2075 is no longer needed. I verified that a clean /t:Rebuild with EnableTrimAnalyzer=true and EnableAotAnalyzer=true produces no new trim warnings.

I left the (int) cast on the boxed enum as is. Unboxing an enum to its underlying type is valid, so it was never part of the bug. It would arguably read better to resolve the nested Utilities.MemoryPressure type and compare against Enum.Parse(pressureType, "High") instead of the literal 2. That is a couple of extra lines and I am happy to follow up if you would prefer it. In the meantime, I left a small comment explaining what that magic 2 is.

Verification

Run locally on linux-x64 with DOTNET_TEST_STRESS=1.

Before:

System.Buffers.Tests  Total: 91, Failed: 1, Skipped: 0

ThreadLocalIsCollectedUnderHighPressure  Fail (0.70s)
  RemoteExecutionException: Remote process failed with an unhandled exception.
  Child exception:
    System.NullReferenceException: Object reference not set to an instance of an object.
       at CollectionTests.<ThreadLocalIsCollectedUnderHighPressure>b__3_0()
          in .../ArrayPool/CollectionTests.cs:line 110

After:

System.Buffers.Tests  Total: 91, Failed: 0, Skipped: 0

ThreadLocalIsCollectedUnderHighPressure  Pass (6.66s)

The loop only exits once GetMemoryPressure() returns High, so the test completing confirms that the reflection now resolves and that the pool genuinely drops the buffer under pressure. Reproduced twice for good measure, at 6.66s and 6.61s.

Without DOTNET_TEST_STRESS the suite is 91 passed / 1 skipped both before and after, so nothing else is affected.

Note for reviewers

The test remains stress gated, so CI will continue to skip it and the local run above is the only evidence that it passes. DOTNET_TEST_STRESS is not set anywhere in the repo's pipelines, so this is true of every stress gated test. Reproducing locally is a single command if you would like to confirm it independently:

DOTNET_TEST_STRESS=1 ./dotnet.sh build src/libraries/System.Runtime/tests/System.Buffers.Tests/System.Buffers.Tests.csproj /t:Test -c Release

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Fixes a stress-gated System.Buffers ArrayPool test that was using broken private reflection by resolving GetMemoryPressure on the correct internal type (System.Buffers.Utilities) and adding assertions to fail fast if reflection breaks again.

Changes:

• Switch reflection lookup from ArrayPool<byte>.Shared.GetType() to typeof(ArrayPool<byte>).Assembly.GetType("System.Buffers.Utilities").
• Add assertions that the internal type and method resolve, avoiding a later NullReferenceException.
• Remove the now-unnecessary #pragma warning disable IL2075 around the broken reflection.

[ApparentlyPlus]

This PR got auto labelled area-VM-coreclr, but the only file it touches is src/libraries/System.Runtime/tests/System.Buffers.Tests/ArrayPool/CollectionTests.cs.

Going by area-owners.md, area-System.Buffers looks like the correct label.

Could someone relabel it so it routes to the right area owners? @jeffhandley, I apologize for the direct ping, I am flagging it since the current label points this at the CoreCLR VM team instead.

[ApparentlyPlus]

@dotnet-policy-service agree

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[ApparentlyPlus]

Applied both inline suggestions by @teo-tsirpanis on 09eed12b21c, minus throwOnError: true since the new test asserts instead of the initializer throwing. The static constructor idea ran into a problem though.

PollingEventFires (another test) and ThreadLocalIsCollectedUnderHighPressure (the test we're fixing) are both ConditionalFacts gated on members of this class, so xunit touches CollectionTests while evaluating their conditions. A throwing type initializer comes back as TargetInvocationException and both get reported as Condition(s) not met, so they would both be silently skipped rather than fail. When I ran outerloop, it failed properly (TypeInitializationException on the two [OuterLoop] tests), but innerloop goes quiet and PollingEventFires stops running altogether.

What I pushed instead keeps the MethodInfo in a static readonly field and adds a small unconditional test asserting the reflection resolves (MemoryPressureHelperIsAvailable). The test reads the field, so initialization happens regardless of beforefieldinit, nothing throws during init, and any API change will now fail in innerloop on every PR rather than only in outerloop, regardless of DOTNET_TEST_STRESS=1.

I went for a single field because splitting the Type out fails trimming with IL2080 (GetMethod on an unannotated Type field). Verified locally by renaming the target method: innerloop gives one isolated failure on the new test and nothing else changes.

@jkotas this deviates from what you approved, and adds a new unconditional test, but I think it is the better trade-off. The alternatives all detect the drift indirectly, and this one fails on the thing it is actually checking.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/libraries/System.Runtime/tests/System.Buffers.Tests/ArrayPool/CollectionTests.cs:110

• s_pressureMethod is nullable and is dereferenced inside the RemoteExecutor delegate. If someone runs only ThreadLocalIsCollectedUnderHighPressure (or if resolution differs in the remote process), this can throw NullReferenceException before producing a useful assertion failure. Capture a non-null local MethodInfo via Assert.NotNull and use it for ReturnType/Invoke to make the failure mode explicit and avoid nullable deref.

                object highPressure = Enum.Parse(s_pressureMethod.ReturnType, "High");

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/libraries/System.Runtime/tests/System.Buffers.Tests/ArrayPool/CollectionTests.cs:88

• PressureMethod is nullable, but ThreadLocalIsCollectedUnderHighPressure dereferences it without any local assertion. The new non-stress Fact helps in full-suite runs, but test execution order (and filtered runs) is not guaranteed, so this can still regress back to a NullReferenceException without a clear diagnostic. Consider making the helper resolution self-validating (assert type + method) and returning a non-null MethodInfo, then have the Fact simply touch it.

        private MethodInfo? PressureMethod =>
            Type.GetType("System.Buffers.Utilities, System.Private.CoreLib")
                ?.GetMethod("GetMemoryPressure", BindingFlags.Static | BindingFlags.NonPublic, Type.EmptyTypes);

        // ThreadLocalIsCollectedUnderHighPressure only runs under DOTNET_TEST_STRESS=1, so without
        // this, the private API it reflects on could change without anyone noticing.
        [Fact]
        public void MemoryPressureHelperIsAvailable()
        {
            Assert.NotNull(PressureMethod);
        }

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.