unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
dotnet/runtime
senior8/10
memory-managementdata-structureserror-handlingtesting-strategy

Fix generic virtual dispatch hang after multiple AssemblyLoadContext unloads

Project: dotnet/runtime | Language: C# | Concepts: memory-management, data-structures, error-handling, testing-strategy | Difficulty: senior
Link: https://github.com/dotnet/runtime/pull/132859

The problem

After unloading multiple collectible AssemblyLoadContexts, the generic virtual dispatch cache could retain stale entries. This happened because the cache flush mechanism failed to recognize the sentinel table, causing new cache entries to be inserted into the shared sentinel instead of a fresh table. As a result, after repeated unloads and address reuse, the runtime could return invalid method pointers, leading to hangs or crashes.

Why it's non-trivial

A naive fix might simply flush or clear the cache, but the bug was caused by a subtle mismatch: the code checked for a sentinel table by comparing table.Length == 2, but the actual sentinel table had a length of 3 (due to an extra element for auxiliary data). This off-by-one error meant the sentinel was never recognized, so stale entries persisted. Fixing this required a precise understanding of the cache's internal structure and careful updates to all relevant checks.

The approach

1. Define a constant for the sentinel table size and use a helper for recognition.

+        // The number of elements in the sentinel table (see _sentinelTable).
+        private const int SENTINEL_TABLE_SIZE = 2;

This makes the sentinel size explicit and reusable.

2. Update the cache constructor assertions to use the new constant, ensuring correct sizing:

-            Debug.Assert(BitOperations.PopCount((uint)initialCacheSize) == 1 && initialCacheSize > 1);
+            Debug.Assert(BitOperations.PopCount((uint)initialCacheSize) == 1 && initialCacheSize > SENTINEL_TABLE_SIZE);

This prevents accidental creation of undersized tables.

3. Allocate the sentinel table using the constant:

-            _sentinelTable = CreateCacheTable(2, throwOnFail: true)!;
+            _sentinelTable = CreateCacheTable(SENTINEL_TABLE_SIZE, throwOnFail: true)!;

This ensures consistency if the sentinel size ever changes.

4. Add and use an IsSentinel helper to recognize the sentinel table correctly:

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static bool IsSentinel(Entry[] table)
+        {
+            // The sentinel is the only table with SENTINEL_TABLE_SIZE elements.
+            // NOTE: the actual array is one element longer, since element 0 is used for aux data.
+            return table.Length == SENTINEL_TABLE_SIZE + 1;
+        }

This abstracts the logic and prevents future off-by-one mistakes.

5. Replace all direct sentinel checks with the helper:

-                if (table.Length == 2)
+                if (IsSentinel(table))

This ensures the sentinel is correctly detected during cache operations.

6. Apply the same pattern to CastCache for consistency:

+        private const int SENTINEL_TABLE_SIZE = 2;
...
-            s_sentinelTable ??= CreateCastCache(2, throwOnFail: true);
+            s_sentinelTable ??= CreateCastCache(SENTINEL_TABLE_SIZE, throwOnFail: true);
...
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static bool IsSentinel(ref int tableData)
+        {
+            // The sentinel is the only table with SENTINEL_TABLE_SIZE elements.
+            return TableMask(ref tableData) == SENTINEL_TABLE_SIZE - 1;
+        }
...
-                if (TableMask(ref tableData) == 1)
+                if (IsSentinel(ref tableData))

This keeps the codebase uniform and avoids similar bugs elsewhere.

7. Add a regression test that loads, invokes, and unloads collectible assemblies, then asserts the cache is empty:

+    [Fact]
+    public static void CallGenericVirtualMethodAcrossUnloads()
+    {
+        ...
+        for (int iteration = 0; iteration < 10; iteration++)
+        {
+            WeakReference context = InvokeAndUnload(payloadPath, iteration);
+            for (int collection = 0; context.IsAlive && collection < 10; collection++)
+            {
+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+            }
+
+            Assert.False(context.IsAlive, $"Context {iteration} did not unload.");
+            VerifyVirtualDispatchCacheIsEmpty();
+        }
+    }

This test ensures that after each unload, the cache is in a clean state and no stale entries remain.

From the review thread

VSadov: @copilot check if CastCache needs similar change for consistency.
We do not need to add a test for CastCache though as it would not cause failures.

Copilot:
Checked both the managed CastCache.cs and the native src/coreclr/vm/castcache.cpp. Neither has the GenericCache bug: their sentinel checks read the aux TableMask (== 1), not the array length, so they correctly recognize the 2-element sentinel. GenericCache was the odd one out because it derives the size from Array.Length.

For consistency I applied the same naming in the managed CastCache.cs only — SENTINEL_TABLE_SIZE constant plus an inlined IsSentinel(ref tableData) used by both checks in TrySet, and the ctor assert now uses the constant. TableMask == SENTINEL_TABLE_SIZE - 1 is constant-folded, so codegen is unchanged. Left the native side alone. Commit b612474f; clr.corelib and clr.nativeaotruntime+clr.nativeaotlibs both build clean.

VSadov: @EgorBo - I think you've recently looked at the cache implementation and should be familiar with how the cache works. Can you review this fix?

[review] VSadov: @copilot can we compare the table size with SENTINEL_TABLE_SIZE + 1 instead - to avoid - 1.
Just in case. This could be used in perf intensive scenarios.

Copilot: Changed IsSentinel to table.Length == SENTINEL_TABLE_SIZE + 1, so there's no subtraction on the hot path. Commit b69c858b.

Takeaways

  • Always centralize "magic numbers" as named constants to avoid subtle mismatches and make future maintenance safer.
  • Encapsulate non-trivial checks (such as sentinel recognition) in helper methods to prevent copy-paste bugs and clarify intent.
  • When fixing a bug in a reusable pattern, audit and update similar code for consistency, even if the bug isn't currently present elsewhere.
  • Regression tests should target the specific invariant being fixed, and avoid assumptions that could produce false positives or negatives if the implementation changes.