From 3cd64558e864db3a7da1873014aa9f9d02e7df65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Wed, 25 Oct 2023 12:09:31 -0400 Subject: [PATCH] [mono] Fix deadlock in static constructor initialization (#93875) * [mono] Fix deadlock in static constructor initialization If two threads (A and B) need to call the static constructors for 3 classes X, Y, Z in this order: Thread A: X, Z, Y Thread B: Y, Z, X where the cctor for X in thread A invokes the cctor for Z and for Y, and the cctor for Y in thread B invokes the cctor for Z and X, then we can get into a situation where A and B both start the cctors for X and Y (so they will be in the type_initialization_hash for those two classes) and then both will try to init Z. In that case it could be that A will be responsible for initializing Z and B will block. Then A could finish initializing Z but B may not have woken up yet (and so it will be in blocked_thread_hash waiting for Z). At that point A (who is at this point already need to init Y) may think that it can wait for B to finish initializing Y. That is we get to `mono_coop_cond_timedwait (&lock->cond, &lock->mutex, timeout_ms)` with "lock" being the lock for `Y`. But in fact thread B will not be able to complete initializing Y because it will attempt to init X next - but meanwhile we did not indicate that thread A is blocked. As a result in thread A the timed wait will eventually timeout. And in this case we need to go back to the top and now correctly detect that A is waiting for Y and B is waiting for X. (At that point there is a cctor deadlock and ECMA rules allow one of the threads to return without calling the cctor) The old code here used to do an infinite wait: while (!lock->done) mono_coop_cond_wait (&lock->cond, &lock->mutex) This cannot succeed because "lock" (in thread A it's the lock for Y) will not be signaled since B (who is supposed to init Y) will instead block on the cctor for X. Fixes https://github.com/dotnet/runtime/issues/93778 * Add test case * remove prototyping log messages * disable mt test on wasm * code review feedback --- src/mono/mono/metadata/object.c | 51 ++++- .../CircularCctorTwoThreadsThreeCC.cs | 182 ++++++++++++++++++ .../CircularCctorTwoThreadsThreeCC.csproj | 9 + src/tests/issues.targets | 3 + 4 files changed, 243 insertions(+), 2 deletions(-) create mode 100644 src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.cs create mode 100644 src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.csproj diff --git a/src/mono/mono/metadata/object.c b/src/mono/mono/metadata/object.c index 898f6df0eaa77b..9cbe2ee44a8350 100644 --- a/src/mono/mono/metadata/object.c +++ b/src/mono/mono/metadata/object.c @@ -457,6 +457,7 @@ mono_runtime_class_init_full (MonoVTable *vtable, MonoError *error) * on this cond var. */ +retry_top: mono_type_initialization_lock (); /* double check... */ if (vtable->initialized) { @@ -506,6 +507,12 @@ mono_runtime_class_init_full (MonoVTable *vtable, MonoError *error) blocked = GUINT_TO_POINTER (MONO_NATIVE_THREAD_ID_TO_UINT (lock->initializing_tid)); while ((pending_lock = (TypeInitializationLock*) g_hash_table_lookup (blocked_thread_hash, blocked))) { if (mono_native_thread_id_equals (pending_lock->initializing_tid, tid)) { + if (mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_TYPE)) { + char* type_name = mono_type_full_name (m_class_get_byval_arg (klass)); + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_TYPE, "Detected deadlock for class .cctor for %s from '%s'", type_name, m_class_get_image (klass)->name); + g_free (type_name); + } + if (!pending_lock->done) { mono_type_initialization_unlock (); goto return_true; @@ -604,9 +611,49 @@ mono_runtime_class_init_full (MonoVTable *vtable, MonoError *error) } else { /* this just blocks until the initializing thread is done */ mono_type_init_lock (lock); - while (!lock->done) - mono_coop_cond_wait (&lock->cond, &lock->mutex); + if (!lock->done) { + int timeout_ms = 500; + int wait_result = mono_coop_cond_timedwait (&lock->cond, &lock->mutex, timeout_ms); + if (wait_result == -1) { + /* timed out - go around again from the beginning. If we got here + * from the "is_blocked = FALSE" case, above (another thread was + * blocked on the current thread, but on a lock that was already + * done but it didn't get to wake up yet), then it might still be + * the case that the current thread cannot proceed even if the other + * thread got to wake up - there might be a new deadlock. We need + * to re-evaluate. + * + * This can happen if two threads A and B need to call the cctors + * for classes X and Y but in opposite orders, and also call a cctor + * for a third class Z. (That is thread A wants to init: X, Z, Y; + * thread B wants to init: Y, Z, X.) In that case, if B is waiting + * for A to finish initializing Z, and A (the current thread ) + * already finished Z and wants to init Y. In A, control will come + * here with "lock" being Y's lock. But we will time out because B + * will see that A is responsible for initializing X and will also + * block. So A is waiting for B to finish Y and B is waiting for A + * to finish X. So the fact that A allowed B to wait for Z to + * finish didn't actually let us make progress. Thread A must go + * around to the top once more and try to init Y - and detect that + * there is now a deadlock between X and Y. + */ + mono_type_init_unlock (lock); + // clean up blocked thread hash and lock refcount. + mono_type_initialization_lock (); + g_hash_table_remove (blocked_thread_hash, GUINT_TO_POINTER (tid)); + gboolean deleted = unref_type_lock (lock); + if (deleted) + g_hash_table_remove (type_initialization_hash, vtable); + mono_type_initialization_unlock (); + goto retry_top; + } else if (wait_result == 0) { + /* Success: we were signaled that the other thread is done. Proceed */ + } else { + g_assert_not_reached (); + } + } mono_type_init_unlock (lock); + g_assert (lock->done); } /* Do cleanup and setting vtable->initialized inside the global lock again */ diff --git a/src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.cs b/src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.cs new file mode 100644 index 00000000000000..cd5b45ca95527f --- /dev/null +++ b/src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.cs @@ -0,0 +1,182 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using System.Threading; + +using Xunit; + +// Regression test for https://github.com/dotnet/runtime/issues/93778 +namespace CircularCctorTwoThreadsThreeCC; + +[Flags] +public enum SlotConstants : int +{ + ZInited = 1, + YInitedFromX = 2, + XInitedFromY = 4, + + Thread1 = 1 << 16, + Thread2 = 2 << 16, +} + +/// X and Y both try to use each other, and also both use Z. +/// We expect to see exactly one thread initialize Z and +/// either X inited from Y or Y inited from X. +public class X +{ + public static X Singleton = new(); + private X() { + Z.Singleton.Ping(); + Y.Singleton?.Pong(); + } + + public void Pong() => Coordinator.Note(SlotConstants.XInitedFromY); +} + +public class Y +{ + public static Y Singleton = new(); + private Y() { + Z.Singleton.Ping(); + X.Singleton?.Pong(); + } + + public void Pong() => Coordinator.Note(SlotConstants.YInitedFromX); +} + +public class Z +{ + public static Z Singleton = new(); + + private Z() { + Coordinator.Note(SlotConstants.ZInited); + } + + public void Ping() { } + +} + +public class Coordinator +{ + [ThreadStatic] + private static SlotConstants t_threadTag; + + private static int s_NextNote; + private static readonly SlotConstants[] Notes = new SlotConstants[12]; + + private static SlotConstants DecorateWithThread(SlotConstants c) + { + return c | t_threadTag; + } + + public static void Note(SlotConstants s) { + int idx = Interlocked.Increment(ref s_NextNote); + idx--; + Notes[idx] = DecorateWithThread (s); + } + + public static Coordinator CreateThread(bool xThenY, SlotConstants threadTag) + { + return new Coordinator(xThenY, threadTag); + } + + public readonly Thread Thread; + private static readonly Barrier s_barrier = new (3); + + private Coordinator(bool xThenY, SlotConstants threadTag) + { + var t = new Thread(() => { + t_threadTag = threadTag; + // Log("started"); + NextPhase(); + // Log("racing"); + DoConstructions(xThenY); + NextPhase(); + // Log("done"); + }); + Thread = t; + t.Start(); + } + + public static void NextPhase() { s_barrier.SignalAndWait(); } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoConstructions(bool xThenY) + { + if (xThenY) { + XCreate(); + } else { + YCreate(); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void XCreate() + { + var _ = X.Singleton; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void YCreate() + { + var _ = Y.Singleton; + } + + public static void Log(string msg) + { + Console.WriteLine ($"{Thread.CurrentThread.ManagedThreadId}: {msg}"); + } + + [Fact] + public static void RunTestCase() + { + var c1 = CreateThread(xThenY: true, threadTag: SlotConstants.Thread1); + var c2 = CreateThread(xThenY: false, threadTag: SlotConstants.Thread2); + // created all threads + NextPhase(); + // racing + NextPhase(); + // done + + // one second should be plenty for all these threads, but it's arbitrary + int threadJoinTimeoutMS = 1000; + var j1 = c1.Thread.Join(threadJoinTimeoutMS); + var j2 = c2.Thread.Join(threadJoinTimeoutMS); + Assert.True(j1); + Assert.True(j2); + // all joined + + // exactly one thread inited Z + Assert.Equal(1, CountNotes(SlotConstants.ZInited)); + // either X was inited or Y, not both. + Assert.Equal(1, Count2Notes(SlotConstants.XInitedFromY, SlotConstants.YInitedFromX)); + } + + private static int CountNotes(SlotConstants mask) + { + int found = 0; + foreach (var note in Notes) { + if ((note & mask) != (SlotConstants)0) { + found++; + } + } + return found; + } + + private static int Count2Notes(SlotConstants mask1, SlotConstants mask2) + { + int found = 0; + foreach (var note in Notes) { + if ((note & mask1) != (SlotConstants)0) { + found++; + } + if ((note & mask2) != (SlotConstants)0) { + found++; + } + } + return found; + } + +} diff --git a/src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.csproj b/src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.csproj new file mode 100644 index 00000000000000..c9936332a86764 --- /dev/null +++ b/src/tests/Loader/classloader/TypeInitialization/CircularCctors/CircularCctorTwoThreadsThreeCC.csproj @@ -0,0 +1,9 @@ + + + true + 0 + + + + + diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 406616c50c5206..3c54496c11966c 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -3221,6 +3221,9 @@ System.Threading.Thread.UnsafeStart not supported + + System.Threading.Thread.ThrowIfNoThreadStart: PlatformNotSupportedException + Could not load legacy Microsoft.Diagnostics.Tools.RuntimeClient