Re: wip-threads-and-fork

[Ludovic Courtès]

Hi!

Andy Wingo <address@hidden> skribis:

> The solution is, besides just avoiding fork() and threads, to take locks
> on all interesting data structures when you fork().  Fortunately there
> are not too many, and most locks are not nested, so it seems to be a
> doable thing.  In wip-threads-and-fork, I added a scm_c_atfork interface
> to define functions to call before and after a fork.

I finally looked in more details at the branch.  In particular, this
part looks great):

7329a5db * use scm_c_atfork_lock_static_mutex for guile's static mutexen
585eb4f7 * add scm_c_atfork_lock_static_mutex
9f6ac5d7 * add atfork interface

What about merging these in stable-2.0?  (With additional comments–e.g.,
to describe ‘scm_c_atfork’–and correct indentation, and without the
weak-set part of 9f6ac5d7.)

However, these patches don’t seem to handle the specific issue that
‘signal_delivery_thread’ could be holding the allocation lock when
‘fork’ runs.  What about doing something along these lines:

diff --git a/libguile/scmsigs.c b/libguile/scmsigs.c
index 86fce0f..e062f24 100644
--- a/libguile/scmsigs.c
+++ b/libguile/scmsigs.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1995,1996,1997,1998,1999,2000,2001, 2002, 2004, 2006, 2007, 
2008, 2009, 2011 Free Software Foundation, Inc.
+/* Copyright (C) 1995,1996,1997,1998,1999,2000,2001, 2002, 2004, 2006, 2007, 
2008, 2009, 2011, 2012 Free Software Foundation, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public License
@@ -183,6 +183,12 @@ read_signal_pipe_data (void * data)
   return NULL;
 }
   
+/* Lock to protect async queuing.  Since `system-async-mark' conses, and
+   thus acquires the GC lock, this lock ensures that the signal thread
+   doesn't call `system-async-mark' in the middle of a `fork'.  */
+static scm_i_pthread_mutex_t signal_async_mutex =
+  SCM_I_PTHREAD_MUTEX_INITIALIZER;
+
 static SCM
 signal_delivery_thread (void *data)
 {
@@ -212,7 +218,11 @@ signal_delivery_thread (void *data)
          h = SCM_SIMPLE_VECTOR_REF (signal_handler_asyncs, sig);
          t = SCM_SIMPLE_VECTOR_REF (signal_handler_threads, sig);
          if (scm_is_true (h))
+           {
+             scm_i_pthread_mutex_lock (&signal_async_mutex);
            scm_system_async_mark_for_thread (h, t);
+             scm_i_pthread_mutex_unlock (&signal_async_mutex);
+           }
        }
       else if (sigdata.n == 0)
        break; /* the signal pipe was closed. */
@@ -713,6 +723,8 @@ scm_init_scmsigs ()
   signal_handler_asyncs = scm_c_make_vector (NSIG, SCM_BOOL_F);
   signal_handler_threads = scm_c_make_vector (NSIG, SCM_BOOL_F);
 
+  scm_c_atfork_lock_static_mutex (&signal_async_mutex);
+
   for (i = 0; i < NSIG; i++)
     {
 #ifdef HAVE_SIGACTION

Thanks,
Ludo’.