Ensure that WaitForPendingFinalizers has seen the expected Full GC count (dotnet#105289)

* Ensure that WaitForPendingFinalizers has seen the expected Full GC

* NativeAOT and some renames

* a testcase

* make the test not unsafe and make OuterLoop

* Use unsigned math when comparing collection ticks

* cast the diff to int when comparing gc ticks

Author: VSadov

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs

@@ -276,7 +276,7 @@ internal static extern unsafe IntPtr RhpCallPropagateExceptionCallback(
 
         // Indicate that the current round of finalizations is complete.
         [DllImport(Redhawk.BaseName)]
-        internal static extern void RhpSignalFinalizationComplete(uint fCount);
+        internal static extern void RhpSignalFinalizationComplete(uint fCount, int observedFullGcCount);
 
         [DllImport(Redhawk.BaseName)]
         internal static extern ulong RhpGetTickCount64();

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs

@@ -29,11 +29,14 @@ public static void ProcessFinalizers()
                 // otherwise memory is low and we should initiate a collection.
                 if (InternalCalls.RhpWaitForFinalizerRequest() != 0)
                 {
+                    int observedFullGcCount = RuntimeImports.RhGetGcCollectionCount(RuntimeImports.RhGetMaxGcGeneration(), false);
                     uint finalizerCount = DrainQueue();
 
-                    // Tell anybody that's interested that the finalization pass is complete (there is a race condition here
-                    // where we might immediately signal a new request as complete, but this is acceptable).
-                    InternalCalls.RhpSignalFinalizationComplete(finalizerCount);
+                    // 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.
+                    // Thus we include the Full GC count that we have certaily observed.
+                    InternalCalls.RhpSignalFinalizationComplete(finalizerCount, observedFullGcCount);
                 }
                 else
                 {

src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp

@@ -94,6 +94,22 @@ EXTERN_C void QCALLTYPE RhInitializeFinalizerThread()
     g_FinalizerEvent.Set();
 }
 
+static int32_t g_fullGcCountSeenByFinalization;
+
+// Indicate that the current round of finalizations is complete.
+EXTERN_C void QCALLTYPE RhpSignalFinalizationComplete(uint32_t fcount, int32_t observedFullGcCount)
+{
+    FireEtwGCFinalizersEnd_V1(fcount, GetClrInstanceId());
+
+    g_fullGcCountSeenByFinalization = observedFullGcCount;
+    g_FinalizerDoneEvent.Set();
+
+    if (YieldProcessorNormalization::IsMeasurementScheduled())
+    {
+        YieldProcessorNormalization::PerformMeasurement();
+    }
+}
+
 EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWait)
 {
     // This must be called via p/invoke rather than RuntimeImport since it blocks and could starve the GC if
@@ -103,13 +119,32 @@ EXTERN_C void QCALLTYPE RhWaitForPendingFinalizers(UInt32_BOOL allowReentrantWai
     // Can't call this from the finalizer thread itself.
     if (ThreadStore::GetCurrentThread() != g_pFinalizerThread)
     {
+        // We may see a completion of finalization cycle that might not see objects that became
+        // F-reachable in recent GCs. In such case we want to wait for a completion of another cycle.
+        // However, since an object cannot be prevented from promoting, one can only rely on Full GCs
+        // to collect unreferenced objects deterministically. Thus we only care about Full GCs here.
+        int desiredFullGcCount =
+            GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration());
+
+    tryAgain:
         // Clear any current indication that a finalization pass is finished and wake the finalizer thread up
         // (if there's no work to do it'll set the done event immediately).
         g_FinalizerDoneEvent.Reset();
         g_FinalizerEvent.Set();
 
         // Wait for the finalizer thread to get back to us.
         g_FinalizerDoneEvent.Wait(INFINITE, false, allowReentrantWait);
+
+        // we use unsigned math here as the collection counts, which are size_t internally,
+        // can in theory overflow an int and wrap around.
+        // unsigned math would have more defined/portable behavior in such case
+        if ((int)((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization) > 0)
+        {
+            // There were some Full GCs happening before we started waiting and possibly not seen by the
+            // last finalization cycle. This is rare, but we need to be sure we have seen those,
+            // so we try one more time.
+            goto tryAgain;
+        }
     }
 }
 
@@ -176,18 +211,6 @@ EXTERN_C UInt32_BOOL QCALLTYPE RhpWaitForFinalizerRequest()
     } while (true);
 }
 
-// Indicate that the current round of finalizations is complete.
-EXTERN_C void QCALLTYPE RhpSignalFinalizationComplete(uint32_t fcount)
-{
-    FireEtwGCFinalizersEnd_V1(fcount, GetClrInstanceId());
-    g_FinalizerDoneEvent.Set();
-
-    if (YieldProcessorNormalization::IsMeasurementScheduled())
-    {
-        YieldProcessorNormalization::PerformMeasurement();
-    }
-}
-
 //
 // The following helpers are special in that they interact with internal GC state or directly manipulate
 // managed references so they're called with a special co-operative p/invoke.

src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/RuntimeImports.cs

@@ -104,5 +104,15 @@ internal static IntPtr RhGetModuleSection(TypeManagerHandle module, ReadyToRunSe
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
         [RuntimeImport(RuntimeLibrary, "RhBulkMoveWithWriteBarrier")]
         internal static extern unsafe void RhBulkMoveWithWriteBarrier(ref byte dmem, ref byte smem, nuint size);
+
+        // Get maximum GC generation number.
+        [MethodImplAttribute(MethodImplOptions.InternalCall)]
+        [RuntimeImport(RuntimeLibrary, "RhGetMaxGcGeneration")]
+        internal static extern int RhGetMaxGcGeneration();
+
+        // Get count of collections so far.
+        [MethodImplAttribute(MethodImplOptions.InternalCall)]
+        [RuntimeImport(RuntimeLibrary, "RhGetGcCollectionCount")]
+        internal static extern int RhGetGcCollectionCount(int generation, bool getSpecialGCCount);
     }
 }

src/coreclr/vm/finalizerthread.cpp

@@ -404,13 +404,15 @@ VOID FinalizerThread::FinalizerThreadWorker(void *args)
         }
         LOG((LF_GC, LL_INFO100, "***** Calling Finalizers\n"));
 
+        int observedFullGcCount =
+            GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration());
         FinalizeAllObjects();
 
         // 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.
-        SignalFinalizationDone();
+        // consider itself satisfied by the drain that just completed.
+        // Thus we include the Full GC count that we have certaily observed.
+        SignalFinalizationDone(observedFullGcCount);
     }
 
     if (s_InitializedFinalizerThreadForPlatform)
@@ -538,10 +540,13 @@ void FinalizerThread::FinalizerThreadCreate()
     }
 }
 
-void FinalizerThread::SignalFinalizationDone()
+static int g_fullGcCountSeenByFinalization;
+
+void FinalizerThread::SignalFinalizationDone(int observedFullGcCount)
 {
     WRAPPER_NO_CONTRACT;
 
+    g_fullGcCountSeenByFinalization = observedFullGcCount;
     hEventFinalizerDone->Set();
 }
 
@@ -555,6 +560,13 @@ void FinalizerThread::FinalizerThreadWait()
     // Can't call this from within a finalized method.
     if (!IsCurrentThreadFinalizer())
     {
+        // We may see a completion of finalization cycle that might not see objects that became
+        // F-reachable in recent GCs. In such case we want to wait for a completion of another cycle.
+        // However, since an object cannot be prevented from promoting, one can only rely on Full GCs
+        // to collect unreferenced objects deterministically. Thus we only care about Full GCs here.
+        int desiredFullGcCount =
+            GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration());
+
         GCX_PREEMP();
 
 #ifdef FEATURE_COMINTEROP
@@ -565,8 +577,8 @@ void FinalizerThread::FinalizerThreadWait()
             g_pRCWCleanupList->CleanupWrappersInCurrentCtxThread();
 #endif // FEATURE_COMINTEROP
 
+    tryAgain:
         hEventFinalizerDone->Reset();
-
         EnableFinalization();
 
         // Under GC stress the finalizer queue may never go empty as frequent
@@ -580,6 +592,18 @@ void FinalizerThread::FinalizerThreadWait()
 
         DWORD status;
         status = hEventFinalizerDone->Wait(INFINITE,TRUE);
+
+        // we use unsigned math here as the collection counts, which are size_t internally,
+        // can in theory overflow an int and wrap around.
+        // unsigned math would have more defined/portable behavior in such case
+        if ((int)((unsigned int)desiredFullGcCount - (unsigned int)g_fullGcCountSeenByFinalization) > 0)
+        {
+            // There were some Full GCs happening before we started waiting and possibly not seen by the
+            // last finalization cycle. This is rare, but we need to be sure we have seen those,
+            // so we try one more time.
+            goto tryAgain;
+        }
+
         _ASSERTE(status == WAIT_OBJECT_0);
     }
 }

src/coreclr/vm/finalizerthread.h

@@ -67,7 +67,7 @@ class FinalizerThread
 
     static void FinalizerThreadWait();
 
-    static void SignalFinalizationDone();
+    static void SignalFinalizationDone(int observedFullGcCount);
 
     static VOID FinalizerThreadWorker(void *args);
     static DWORD WINAPI FinalizerThreadStart(void *args);

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs

@@ -7,6 +7,7 @@
 using System.Runtime.InteropServices;
 using System.Diagnostics;
 using System.Threading;
+using System.Threading.Tasks;
 using System.Runtime;
 using Microsoft.DotNet.RemoteExecutor;
 using Xunit;
@@ -292,6 +293,55 @@ private class TestObject
             }
         }
 
+        [OuterLoop]
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
+        public static void WaitForPendingFinalizersRaces()
+        {
+            Task.Run(Test);
+            Task.Run(Test);
+            Task.Run(Test);
+            Task.Run(Test);
+            Task.Run(Test);
+            Task.Run(Test);
+            Test();
+
+            static void Test()
+            {
+                for (int i = 0; i < 20000; i++)
+                {
+                    BoxedFinalized flag = new BoxedFinalized();
+                    MakeAndNull(flag);
+                    GC.Collect();
+                    GC.WaitForPendingFinalizers();
+                    Assert.True(flag.finalized);
+                }
+            }
+
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            static void MakeAndNull(BoxedFinalized flag)
+            {
+                var deadObj = new TestObjectWithFinalizer(flag);
+                // it's dead here
+            };
+        }
+
+        class BoxedFinalized
+        {
+            public bool finalized;
+        }
+
+        class TestObjectWithFinalizer
+        {
+            BoxedFinalized _flag;
+
+            public TestObjectWithFinalizer(BoxedFinalized flag)
+            {
+                _flag = flag;
+            }
+
+            ~TestObjectWithFinalizer() => _flag.finalized = true;
+        }
+
         [Fact]
         public static void SuppressFinalizer_NullObject_ThrowsArgumentNullException()
         {