Go patch committed: Avoid race condition shutting down thread

Go patch committed: Avoid race condition shutting down thread

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

I spotted a theoretical race condition when shutting down a thread.  If
a garbage collection occurs at the very end of existing the thread, the
garbage collector and the thread could collide on manipulating the heap.
This patch fixes the problem by moving the thread exiting manipulations
within the lock held over all threads.  Bootstrapped and ran Go
testsuite on x86_64-unknown-linux-gnu.  Committed to mainline.

Ian


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1801 bytes --]

diff -r 6d06701000fc libgo/runtime/go-go.c
--- a/libgo/runtime/go-go.c	Mon Mar 07 13:38:09 2011 -0800
+++ b/libgo/runtime/go-go.c	Mon Mar 07 13:49:35 2011 -0800
@@ -92,18 +92,15 @@
   if (list_entry->next != NULL)
     list_entry->next->prev = list_entry->prev;
 
+  /* This will look runtime_mheap as needed.  */
   runtime_MCache_ReleaseAll (mcache);
 
-  /* As soon as we release this look, a GC could run.  Since this
-     thread is no longer on the list, the GC will not find our M
-     structure, so it could get freed at any time.  That means that
-     any code from here to thread exit must not assume that the m is
-     valid.  */
-  m = NULL;
-
-  i = pthread_mutex_unlock (&__go_thread_ids_lock);
-  __go_assert (i == 0);
-
+  /* This should never deadlock--there shouldn't be any code that
+     holds the runtime_mheap lock when locking __go_thread_ids_lock.
+     We don't want to do this after releasing __go_thread_ids_lock
+     because it will mean that the garbage collector might run, and
+     the garbage collector does not try to lock runtime_mheap in all
+     cases since it knows it is running single-threaded.  */
   runtime_lock (&runtime_mheap);
   mstats.heap_alloc += mcache->local_alloc;
   mstats.heap_objects += mcache->local_objects;
@@ -111,6 +108,16 @@
   runtime_FixAlloc_Free (&runtime_mheap.cachealloc, mcache);
   runtime_unlock (&runtime_mheap);
 
+  /* As soon as we release this look, a GC could run.  Since this
+     thread is no longer on the list, the GC will not find our M
+     structure, so it could get freed at any time.  That means that
+     any code from here to thread exit must not assume that m is
+     valid.  */
+  m = NULL;
+
+  i = pthread_mutex_unlock (&__go_thread_ids_lock);
+  __go_assert (i == 0);
+
   free (list_entry);
 }