Test failure: System.Tests.GCTests.KeepAlive_Null

[v-wenyuxu]

Failed in: runtime-coreclr libraries-jitstressregs 20240630.1

Failed tests:

net9.0-linux-Release-x64-jitstressregs0x80-Ubuntu.2204.Amd64.Open
    - System.Tests.GCTests.KeepAlive_Null

Error message:

 Assert.True() Failure
Expected: True
Actual:   False

Stack trace:

   at System.Tests.GCTests.KeepAliveNullTest.Run() in /_/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs:line 211
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs:line 57

[EgorBo]

Doesn't look to be jitstress specific, fails on:

using System.Runtime.CompilerServices;
using Xunit;

class KeepAliveNullTest
{
    private static void Main()
    {
        for (int i = 0; i < 200000; i++)
        {
            TestObject.Finalized = false;
            MakeAndNull();
            GC.Collect();
            GC.WaitForPendingFinalizers();
            Assert.True(TestObject.Finalized);
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void MakeAndNull()
    {
        var deadObj = new TestObject();
        // it's dead here
    }

    public class TestObject
    {
        public static bool Finalized { get; set; }

        ~TestObject() => Finalized = true;
    }
}

with DOTNET_TieredCompilation=0. @dotnet/gc @VSadov any recent changes in GC could break it? It used to pass..

[dotnet-policy-service]

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

[VSadov]

Unless [MethodImpl(MethodImplOptions.NoInlining)] is not respected or the allocation of the object is somehow optimized away, I do not see how JIT or root reporting could cause any changes in behavior here.

This is a very simple case. The object gets unreachable and GC.Collect() should see that.

Could it be that NoInlining is not working?

[EgorBo]

Unless [MethodImpl(MethodImplOptions.NoInlining)] is not respected or the allocation of the object is somehow optimized away, I do not see how JIT or root reporting could cause any changes in behavior here.

This is a very simple case. The object gets unreachable and GC.Collect() should see that.

Could it be that NoInlining is not working?

I've just checked - both NoInlining is correctly respected and the allocation is not optimized away (we normally do remove such allocations unless their classes have finalizers). So sounds like some change in GC's behavior

[EgorBo]

Seems like the DATAS is the culprit, the test doesn't fail with DOTNET_GCDynamicAdaptationMode=0

[jakobbotsch]

My initial bisection suggests f69972f...d2dbdd0, though the repro is intermittent so it's possible I just haven't seen it repro with f69972f yet. In any case if that's the range #103501 looks most interesting, cc @jkotas

[jkotas]

I do not think that the problem is with GC not collecting the object. I think the problem is in the WaitForPendingFinalizers() synchronization. If I change the loop to be:

        for (int i = 0; i < 20000000; i++)
        {
            TestObject.Finalized = false;
            MakeAndNull();
            GC.Collect();
            GC.WaitForPendingFinalizers();
            if (!TestObject.Finalized) Console.WriteLine("FAIL");
            if (!TestObject.Finalized) Console.WriteLine("FAIL2");
         }

I never see FAIL + FAIL2 printed. I always only see FAIL printed. It suggests that the problem is in GC.WaitForPendingFinalizers returning too early before the finalizer got a chance to run.

The code has a comment about such race condition and considers it acceptable:

runtime/src/coreclr/vm/finalizerthread.cpp

Lines 409 to 412 in 104e88d

	// Anyone waiting to drain the Q can now wake up. Note that there is a
	// race in that another thread starting a drain, as we leave a drain, may
	// consider itself satisfied by the drain that just completed. This is
	// acceptable.

#103501

This change made finalization loop more efficient. It makes any race conditions related to finalization more likely to show up.