[runtime] Rework abort deferring

The problem with previous implementation was that we were not able to account for two pending exceptions at the same time, since when we execute an interruption request we clear all the state and there is no clear way to account for two exceptions at the same time. This means that, if we have a pending abort in a cctor, all exception handling inside the cctor will be broken (for the provided test case we were only throwing the cctor internal exception outside of it).

In order to fix this, we split into two types of interruptions SYNC and ASYNC (in order to separate mainly between abort and pending exceptions). Sync interruptions are interruptions requested by the thread itself (pending exceptions and self aborts) and they are processed as soon as possible. Async interruptions are requested by other threads (abort, interrupt, suspend) and we defer them as we were doing before (we have a deferred interruption if we have an async exception and abort block count is non zero).

diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c
index e31e0bbd4e8025165bb8cda75eb496aec531ba51..2ca989bea0fe9e62f10294ff76257355db71ea00 100644 (file)
--- a/mono/metadata/threads.c
+++ b/mono/metadata/threads.c
@@ -219,9 +219,18 @@ get_next_managed_thread_id (void)
        return InterlockedIncrement (&managed_thread_id_counter);
 }
 
+/*
+ * We separate interruptions/exceptions into either sync (they can be processed anytime,
+ * normally as soon as they are set, and are set by the same thread) and async (they can't
+ * be processed inside abort protected blocks and are normally set by other threads). We
+ * can have both a pending sync and async interruption. In this case, the sync exception is
+ * processed first. Since we clean sync flag first, mono_thread_execute_interruption must
+ * also handle all sync type exceptions before the async type exceptions.
+ */
 enum {
-       INTERRUPT_REQUESTED_BIT = 0x1,
-       INTERRUPT_REQUEST_DEFERRED_BIT = 0x2,
+       INTERRUPT_SYNC_REQUESTED_BIT = 0x1,
+       INTERRUPT_ASYNC_REQUESTED_BIT = 0x2,
+       INTERRUPT_REQUESTED_MASK = 0x3,
        ABORT_PROT_BLOCK_SHIFT = 2,
        ABORT_PROT_BLOCK_BITS = 8,
        ABORT_PROT_BLOCK_MASK = (((1 << ABORT_PROT_BLOCK_BITS) - 1) << ABORT_PROT_BLOCK_SHIFT)
@@ -234,149 +243,111 @@ mono_thread_get_abort_prot_block_count (MonoInternalThread *thread)
        return (state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT;
 }
 
-static void
-verify_thread_state (gsize state)
-{
-       //can't have both INTERRUPT_REQUESTED_BIT and INTERRUPT_REQUEST_DEFERRED_BIT set at the same time
-       g_assert ((state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT)) != (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT));
-
-       //XXX This would be nice to be true, but can happen due to self-aborts (and possibly set-pending-exception)
-       //if prot_count > 0, INTERRUPT_REQUESTED_BIT must never be set
-       // int prot_count = (state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT;
-       // g_assert (!(prot_count > 0 && (state & INTERRUPT_REQUESTED_BIT)));
-}
-
 void
 mono_threads_begin_abort_protected_block (void)
 {
        MonoInternalThread *thread = mono_thread_internal_current ();
        gsize old_state, new_state;
+       int new_val;
        do {
                old_state = thread->thread_state;
-               verify_thread_state (old_state);
-
-               int new_val = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT) + 1;
-
-               new_state = 0;
-               if (old_state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT)) {
-                       new_state |= INTERRUPT_REQUEST_DEFERRED_BIT;
-               }
 
+               new_val = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT) + 1;
                //bounds check abort_prot_count
                g_assert (new_val > 0);
                g_assert (new_val < (1 << ABORT_PROT_BLOCK_BITS));
-               new_state |= new_val << ABORT_PROT_BLOCK_SHIFT;
 
+               new_state = old_state + (1 << ABORT_PROT_BLOCK_SHIFT);
        } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
 }
 
-gboolean
-mono_threads_end_abort_protected_block (void)
+static gboolean
+mono_thread_state_has_interruption (gsize state)
 {
-       MonoInternalThread *thread = mono_thread_internal_current ();
-       gsize old_state, new_state;
-       do {
-               old_state = thread->thread_state;
-               verify_thread_state (old_state);
-
-               int new_val = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT) - 1;
-               new_state = 0;
-
-               if (old_state & INTERRUPT_REQUEST_DEFERRED_BIT) {
-                       if (new_val == 0)
-                               new_state |= INTERRUPT_REQUESTED_BIT;
-                       else
-                               new_state |= INTERRUPT_REQUEST_DEFERRED_BIT;
-               }
+       /* pending exception, self abort */
+       if (state & INTERRUPT_SYNC_REQUESTED_BIT)
+               return TRUE;
 
-               //bounds check abort_prot_count
-               g_assert (new_val >= 0);
-               g_assert (new_val < (1 << ABORT_PROT_BLOCK_BITS));
-               new_state |= new_val << ABORT_PROT_BLOCK_SHIFT;
+       /* abort, interruption, suspend */
+       if ((state & INTERRUPT_ASYNC_REQUESTED_BIT) && !(state & ABORT_PROT_BLOCK_MASK))
+               return TRUE;
 
-       } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
-       return (new_state & INTERRUPT_REQUESTED_BIT) == INTERRUPT_REQUESTED_BIT;
+       return FALSE;
 }
 
-
-//Don't use this function, use inc/dec below
-static void
-mono_thread_abort_prot_block_count_add (MonoInternalThread *thread, int val)
+gboolean
+mono_threads_end_abort_protected_block (void)
 {
+       MonoInternalThread *thread = mono_thread_internal_current ();
        gsize old_state, new_state;
        do {
                old_state = thread->thread_state;
-               verify_thread_state (old_state);
 
-               int new_val = val + ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT);
                //bounds check abort_prot_count
+               int new_val = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT) - 1;
                g_assert (new_val >= 0);
                g_assert (new_val < (1 << ABORT_PROT_BLOCK_BITS));
-               new_state = (old_state & ~ABORT_PROT_BLOCK_MASK) | (new_val << ABORT_PROT_BLOCK_SHIFT);
 
+               new_state = old_state - (1 << ABORT_PROT_BLOCK_SHIFT);
        } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
-}
 
-static void
-mono_thread_inc_abort_prot_block_count (MonoInternalThread *thread)
-{
-       mono_thread_abort_prot_block_count_add (thread, 1);
-}
-
-static void
-mono_thread_dec_abort_prot_block_count (MonoInternalThread *thread)
-{
-       mono_thread_abort_prot_block_count_add (thread, -1);
+       return mono_thread_state_has_interruption (new_state);
 }
 
 static gboolean
 mono_thread_get_interruption_requested (MonoInternalThread *thread)
 {
        gsize state = thread->thread_state;
-       return (state & INTERRUPT_REQUESTED_BIT) == INTERRUPT_REQUESTED_BIT;
+
+       return mono_thread_state_has_interruption (state);
 }
 
-/* Returns TRUE is there was a state change */
+/*
+ * Returns TRUE is there was a state change
+ * We clear a single interruption request, sync has priority.
+ */
 static gboolean
 mono_thread_clear_interruption_requested (MonoInternalThread *thread)
 {
        gsize old_state, new_state;
        do {
                old_state = thread->thread_state;
-               verify_thread_state (old_state);
 
-               //Already cleared
-               if (!(old_state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT)))
+               // no interruption to process
+               if (!(old_state & INTERRUPT_SYNC_REQUESTED_BIT) &&
+                               (!(old_state & INTERRUPT_ASYNC_REQUESTED_BIT) || (old_state & ABORT_PROT_BLOCK_MASK)))
                        return FALSE;
-               new_state = old_state & ~(INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT);
+
+               if (old_state & INTERRUPT_SYNC_REQUESTED_BIT)
+                       new_state = old_state & ~INTERRUPT_SYNC_REQUESTED_BIT;
+               else
+                       new_state = old_state & ~INTERRUPT_ASYNC_REQUESTED_BIT;
        } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
        return TRUE;
 }
 
-/* Returns TRUE is there was a state change */
+/* Returns TRUE is there was a state change and the interruption can be processed */
 static gboolean
 mono_thread_set_interruption_requested (MonoInternalThread *thread)
 {
        //always force when the current thread is doing it to itself.
-       gboolean force_interrupt = thread == mono_thread_internal_current ();
+       gboolean sync = thread == mono_thread_internal_current ();
        gsize old_state, new_state;
        do {
                old_state = thread->thread_state;
-               verify_thread_state (old_state);
 
-               int prot_count = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT);
                //Already set
-               if (old_state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT))
+               if ((sync && (old_state & INTERRUPT_SYNC_REQUESTED_BIT)) ||
+                               (!sync && (old_state & INTERRUPT_ASYNC_REQUESTED_BIT)))
                        return FALSE;
 
-               //If there's an outstanding prot block, we queue it
-               if (prot_count && !force_interrupt) {
-                       new_state = old_state | INTERRUPT_REQUEST_DEFERRED_BIT;
-               } else
-                       new_state = old_state | INTERRUPT_REQUESTED_BIT;
+               if (sync)
+                       new_state = old_state | INTERRUPT_SYNC_REQUESTED_BIT;
+               else
+                       new_state = old_state | INTERRUPT_ASYNC_REQUESTED_BIT;
        } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
 
-       return (new_state & INTERRUPT_REQUESTED_BIT) == INTERRUPT_REQUESTED_BIT;
+       return sync || !(new_state & ABORT_PROT_BLOCK_MASK);
 }
 
 static inline MonoNativeThreadId

diff --git a/mono/tests/abort-cctor-2.cs b/mono/tests/abort-cctor-2.cs
new file mode 100644 (file)
index 0000000..f1e7548
--- /dev/null
+++ b/mono/tests/abort-cctor-2.cs
@@ -0,0 +1,54 @@
+using System;
+using System.Threading;
+
+public class Critical {
+       static Critical ()
+       {
+               Program.mre1.Set ();
+               Program.mre2.WaitOne ();
+               try {
+                       throw new Exception ();
+               } catch (Exception) {
+                       Console.WriteLine ("Catched exception in cctor");
+                       Program.catched_exception = true;
+               }
+       }
+}
+
+
+public class Program {
+       public static ManualResetEvent mre1 = new ManualResetEvent (false);
+       public static ManualResetEvent mre2 = new ManualResetEvent (false);
+
+       public static bool catched_exception, catched_abort;
+
+       public static int Main (string[] args)
+       {
+               Thread thread = new Thread (DoStuff);
+               thread.Start ();
+
+               mre1.WaitOne ();
+               thread.Abort ();
+               mre2.Set ();
+
+               thread.Join ();
+
+               if (!catched_exception)
+                       Environment.Exit (1);
+               if (!catched_abort)
+                       Environment.Exit (2);
+
+               Console.WriteLine ("done, all things good");
+               return 0;
+       }
+
+       public static void DoStuff ()
+       {
+               try {
+                       new Critical ();
+               } catch (ThreadAbortException) {
+                       Console.WriteLine ("Catched thread abort");
+                       Program.catched_abort = true;
+               }
+       }
+}

diff --git a/mono/tests/abort-cctor.cs b/mono/tests/abort-cctor.cs
index 19c494c9d5c3b571978f93a87466a4b075fe24a7..c3520ba92cf8204cd1169ad82c8237a15146b760 100644 (file)
--- a/mono/tests/abort-cctor.cs
+++ b/mono/tests/abort-cctor.cs
@@ -319,6 +319,47 @@ class Driver
        }
 
 
+       class StaticConstructor5 {
+               public static bool catched_exception = false;
+               static StaticConstructor5 ()
+               {
+                       Driver.mre1.Set ();
+                       Driver.mre2.WaitOne ();
+                       try {
+                               throw new Exception ();
+                       } catch (Exception) {
+                               Console.WriteLine ("Catched exception in cctor");
+                               catched_exception = true;
+                       }
+               }
+       }
+
+       static void Test5 ()
+       {
+               bool catched_abort = false;
+               Driver.mre1.Reset ();
+               Driver.mre2.Reset ();
+               Thread thread = new Thread (() => {
+                                       try {
+                                               new StaticConstructor5 ();
+                                       } catch (ThreadAbortException) {
+                                               Console.WriteLine ("Catched thread abort");
+                                               catched_abort = true;
+                                       }
+                               });
+               thread.Start ();
+
+               Driver.mre1.WaitOne ();
+               thread.Abort ();
+               Driver.mre2.Set ();
+
+               thread.Join ();
+
+               if (!StaticConstructor5.catched_exception)
+                       Environment.Exit (14);
+               if (!catched_abort)
+                       Environment.Exit (15);
+       }
 
        public static int Main ()
        {
@@ -326,7 +367,8 @@ class Driver
                Test2 ();
                Test3 ();
                Test4 ();
+               Test5 ();
                Console.WriteLine ("done, all things good");
                return 0;
        }
-}
\ No newline at end of file
+}