Re: Does Guile have a thread limit?

[Taylan Ulrich Bayirli/Kammer]

address@hidden (Taylan Ulrich "Bayırlı/Kammer") writes:

> While C code using up many FDs and then calling scm_with_guile() might
> abort --which should be a rare case--, `call-with-new-thread' neatly
> raises an exception ... almost.  In the REPL, when I repeatedly enter
> our test-case snippet of launching 1k sleeping threads, I manage to
> make Guile hang.  In case anyone's interested, here's the backtraces
> of all threads when I attach with GDB: http://sprunge.us/VZMY

I solved this now, the problem was that I didn't think about cleanup at
all after a failed thread initialization; though in this case the only
thing that needed to be done was to call GC_unregister_my_thread.

Does anyone have time to review this patch?  It passes `make check' and
neatly throws an exception when a thread fails to initialize due to
running out of FDs.  (Well, in the REPL, one gets flooded with readline
errors when that fails to acquire an FD because the succeeding threads
ate up all the available ones, but that's a different issue.)

>From c4ff5443b85e9840b35b0d0a7e8a70cbbba35f25 Mon Sep 17 00:00:00 2001
From: Taylan Ulrich B <address@hidden>
Date: Thu, 10 Apr 2014 17:29:28 +0200
Subject: [PATCH] Handle thread initialization failure

* libguile/init.h (scm_init_guile): Return int for errors.
* libguile/threads.h (scm_with_guile_safe): New function.
(scm_threads_prehistory): Return int for error.
(scm_init_threads): Return int for errors.

* libguile/threads.c (guilify_self_1): Return int for errors.
(guilify_self_2): Return int for errors.
(scm_i_init_thread_for_guile): Return -1 on failure.
(scm_init_guile): Return int for errors.
(with_guile_args): Add `int err' field.
(with_guile_and_parent): Handle thread initialization failure.
(scm_i_with_guile_and_parent): Handle `with_guile_and_parent' error.
(scm_with_guile): Abort on failure (as before, just explicitly).
(scm_with_guile_safe): Non-aborting variant, takes `int *err'.
(launch_data): Add `int err' field.
(launch_thread): Handle `scm_i_with_guile_and_parent' error.
(call-with-new-thread): Signal thread initialization failure.
(spawn_data): Add `int err' field.
(spawn_thread): Signal thread initialization failure.
(scm_threads_prehistory): Return int for errors.
(scm_init_threads): Return int for errors.
---
 libguile/init.h    |   2 +-
 libguile/threads.c | 110 +++++++++++++++++++++++++++++++++++++++++------------
 libguile/threads.h |   5 ++-
 3 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/libguile/init.h b/libguile/init.h
index bc6cddf..f529c4f 100644
--- a/libguile/init.h
+++ b/libguile/init.h
@@ -30,7 +30,7 @@
 SCM_INTERNAL scm_i_pthread_mutex_t scm_i_init_mutex;
 SCM_API int scm_initialized_p;
 
-SCM_API void scm_init_guile (void);
+SCM_API int scm_init_guile (void);
 
 SCM_API void scm_boot_guile (int argc, char **argv,
                             void (*main_func) (void *closure,
diff --git a/libguile/threads.c b/libguile/threads.c
index bcf1e0d..f361f1d 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -392,7 +392,7 @@ scm_i_reset_fluid (size_t n)
 
 /* Perform first stage of thread initialisation, in non-guile mode.
  */
-static void
+static int
 guilify_self_1 (struct GC_stack_base *base)
 {
   scm_i_thread t;
@@ -432,10 +432,7 @@ guilify_self_1 (struct GC_stack_base *base)
   t.vp = NULL;
 
   if (pipe2 (t.sleep_pipe, O_CLOEXEC) != 0)
-    /* FIXME: Error conditions during the initialization phase are handled
-       gracelessly since public functions such as `scm_init_guile ()'
-       currently have type `void'.  */
-    abort ();
+    return -1;
 
   scm_i_pthread_mutex_init (&t.admin_mutex, NULL);
   t.canceled = 0;
@@ -465,11 +462,13 @@ guilify_self_1 (struct GC_stack_base *base)
 
     GC_enable ();
   }
+
+  return 0;
 }
 
 /* Perform second stage of thread initialisation, in guile mode.
  */
-static void
+static int
 guilify_self_2 (SCM parent)
 {
   scm_i_thread *t = SCM_I_CURRENT_THREAD;
@@ -501,6 +500,8 @@ guilify_self_2 (SCM parent)
 
   /* See note in finalizers.c:queue_finalizer_async().  */
   GC_invoke_finalizers ();
+
+  return 0;
 }
 
 
@@ -681,7 +682,7 @@ init_thread_key (void)
    which case the default dynamic state is used.
 
    Returns zero when the thread was known to guile already; otherwise
-   return 1.
+   return 1 for success, -1 for failure.
 
    Note that it could be the case that the thread was known
    to Guile, but not in guile mode (because we are within a
@@ -731,21 +732,25 @@ scm_i_init_thread_for_guile (struct GC_stack_base *base, 
SCM parent)
           GC_register_my_thread (base);
 #endif
 
-         guilify_self_1 (base);
-         guilify_self_2 (parent);
+          if ( (guilify_self_1 (base) != 0) ||
+               (guilify_self_2 (parent) != 0))
+            {
+              GC_unregister_my_thread ();
+              return -1;
+            }
        }
       return 1;
     }
 }
 
-void
+int
 scm_init_guile ()
 {
   struct GC_stack_base stack_base;
   
   if (GC_get_stack_base (&stack_base) == GC_SUCCESS)
-    scm_i_init_thread_for_guile (&stack_base,
-                                 scm_i_default_dynamic_state);
+    return scm_i_init_thread_for_guile (&stack_base,
+                                        scm_i_default_dynamic_state);
   else
     {
       fprintf (stderr, "Failed to get stack base for current thread.\n");
@@ -758,6 +763,7 @@ struct with_guile_args
   GC_fn_type func;
   void *data;
   SCM parent;
+  int err;
 };
 
 static void *
@@ -777,6 +783,11 @@ with_guile_and_parent (struct GC_stack_base *base, void 
*data)
   struct with_guile_args *args = data;
 
   new_thread = scm_i_init_thread_for_guile (base, args->parent);
+  if (new_thread < 0)
+    {
+      args->err = new_thread;
+      return NULL;
+    }
   t = SCM_I_CURRENT_THREAD;
   if (new_thread)
     {
@@ -818,22 +829,44 @@ with_guile_and_parent (struct GC_stack_base *base, void 
*data)
 }
 
 static void *
-scm_i_with_guile_and_parent (void *(*func)(void *), void *data, SCM parent)
+scm_i_with_guile_and_parent (void *(*func)(void *), void *data, SCM parent,
+                             int *err)
 {
   struct with_guile_args args;
+  void *result;
 
   args.func = func;
   args.data = data;
   args.parent = parent;
-  
-  return GC_call_with_stack_base (with_guile_and_parent, &args);
+  args.err = 0;
+
+  result = GC_call_with_stack_base (with_guile_and_parent, &args);
+  if (err)
+    *err = args.err;
+  return result;
 }
 
 void *
 scm_with_guile (void *(*func)(void *), void *data)
 {
+  void *result;
+  int err = 0;
+
+  result = scm_i_with_guile_and_parent (func, data,
+                                        scm_i_default_dynamic_state,
+                                        &err);
+  if (err)
+    abort ();
+
+  return result;
+}
+
+void *
+scm_with_guile_safe (void *(*func)(void *), void *data, int *err)
+{
   return scm_i_with_guile_and_parent (func, data,
-                                     scm_i_default_dynamic_state);
+                                      scm_i_default_dynamic_state,
+                                      err);
 }
 
 void *
@@ -865,6 +898,7 @@ typedef struct {
   SCM thread;
   scm_i_pthread_mutex_t mutex;
   scm_i_pthread_cond_t cond;
+  int err;
 } launch_data;
 
 static void *
@@ -893,8 +927,16 @@ static void *
 launch_thread (void *d)
 {
   launch_data *data = (launch_data *)d;
+  int err = 0;
   scm_i_pthread_detach (scm_i_pthread_self ());
-  scm_i_with_guile_and_parent (really_launch, d, data->parent);
+  scm_i_with_guile_and_parent (really_launch, d, data->parent, &err);
+  if (err)
+    {
+      scm_i_pthread_mutex_lock (&data->mutex);
+      data->err = err;
+      scm_i_pthread_cond_signal (&data->cond);
+      scm_i_pthread_mutex_unlock (&data->mutex);
+    }
   return NULL;
 }
 
@@ -927,6 +969,7 @@ SCM_DEFINE (scm_call_with_new_thread, 
"call-with-new-thread", 1, 1, 0,
   data.thread = SCM_BOOL_F;
   scm_i_pthread_mutex_init (&data.mutex, NULL);
   scm_i_pthread_cond_init (&data.cond, NULL);
+  data.err = 0;
 
   scm_i_scm_pthread_mutex_lock (&data.mutex);
   err = scm_i_pthread_create (&id, NULL, launch_thread, &data);
@@ -937,11 +980,14 @@ SCM_DEFINE (scm_call_with_new_thread, 
"call-with-new-thread", 1, 1, 0,
       scm_syserror (NULL);
     }
 
-  while (scm_is_false (data.thread))
+  while (scm_is_false (data.thread) && data.err == 0)
     scm_i_scm_pthread_cond_wait (&data.cond, &data.mutex);
 
   scm_i_pthread_mutex_unlock (&data.mutex);
 
+  if (data.err)
+    SCM_MISC_ERROR ("could not initialize thread", SCM_EOL);
+
   return data.thread;
 }
 #undef FUNC_NAME
@@ -955,6 +1001,7 @@ typedef struct {
   SCM thread;
   scm_i_pthread_mutex_t mutex;
   scm_i_pthread_cond_t cond;
+  int err;
 } spawn_data;
 
 static void *
@@ -986,8 +1033,16 @@ static void *
 spawn_thread (void *d)
 {
   spawn_data *data = (spawn_data *)d;
+  int err = 0;
   scm_i_pthread_detach (scm_i_pthread_self ());
-  scm_i_with_guile_and_parent (really_spawn, d, data->parent);
+  scm_i_with_guile_and_parent (really_spawn, d, data->parent, &err);
+  if (err)
+    {
+      scm_i_pthread_mutex_lock (&data->mutex);
+      data->err = err;
+      scm_i_pthread_cond_signal (&data->cond);
+      scm_i_pthread_mutex_unlock (&data->mutex);
+    }
   return NULL;
 }
 
@@ -1007,6 +1062,7 @@ scm_spawn_thread (scm_t_catch_body body, void *body_data,
   data.thread = SCM_BOOL_F;
   scm_i_pthread_mutex_init (&data.mutex, NULL);
   scm_i_pthread_cond_init (&data.cond, NULL);
+  data.err = 0;
 
   scm_i_scm_pthread_mutex_lock (&data.mutex);
   err = scm_i_pthread_create (&id, NULL, spawn_thread, &data);
@@ -1017,11 +1073,14 @@ scm_spawn_thread (scm_t_catch_body body, void 
*body_data,
       scm_syserror (NULL);
     }
 
-  while (scm_is_false (data.thread))
+  while (scm_is_false (data.thread) && data.err == 0)
     scm_i_scm_pthread_cond_wait (&data.cond, &data.mutex);
 
   scm_i_pthread_mutex_unlock (&data.mutex);
 
+  if (data.err)
+    scm_misc_error (NULL, "could not spawn thread", SCM_EOL);
+
   assert (SCM_I_IS_THREAD (data.thread));
 
   return data.thread;
@@ -2060,7 +2119,7 @@ scm_i_pthread_mutex_t scm_i_misc_mutex;
 pthread_mutexattr_t scm_i_pthread_mutexattr_recursive[1];
 #endif
 
-void
+int
 scm_threads_prehistory (void *base)
 {
 #if SCM_USE_PTHREAD_THREADS
@@ -2079,14 +2138,14 @@ scm_threads_prehistory (void *base)
                 GC_MAKE_PROC (GC_new_proc (thread_mark), 0),
                 0, 1);
 
-  guilify_self_1 ((struct GC_stack_base *) base);
+  return guilify_self_1 ((struct GC_stack_base *) base);
 }
 
 scm_t_bits scm_tc16_thread;
 scm_t_bits scm_tc16_mutex;
 scm_t_bits scm_tc16_condvar;
 
-void
+int
 scm_init_threads ()
 {
   scm_tc16_thread = scm_make_smob_type ("thread", sizeof (scm_i_thread));
@@ -2100,10 +2159,13 @@ scm_init_threads ()
   scm_set_smob_print (scm_tc16_condvar, fat_cond_print);
 
   scm_i_default_dynamic_state = SCM_BOOL_F;
-  guilify_self_2 (SCM_BOOL_F);
+  if (guilify_self_2 (SCM_BOOL_F) != 0)
+    return -1;
   threads_initialized_p = 1;
 
   dynwind_critical_section_mutex = scm_make_recursive_mutex ();
+
+  return 0;
 }
 
 void
diff --git a/libguile/threads.h b/libguile/threads.h
index 6b85baf..60d6a64 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -137,10 +137,11 @@ SCM_API SCM scm_spawn_thread (scm_t_catch_body body, void 
*body_data,
 
 SCM_API void *scm_without_guile (void *(*func)(void *), void *data);
 SCM_API void *scm_with_guile (void *(*func)(void *), void *data);
+SCM_API void *scm_with_guile_safe (void *(*func)(void *), void *data, int 
*err);
 
 SCM_INTERNAL void scm_i_reset_fluid (size_t);
-SCM_INTERNAL void scm_threads_prehistory (void *);
-SCM_INTERNAL void scm_init_threads (void);
+SCM_INTERNAL int scm_threads_prehistory (void *);
+SCM_INTERNAL int scm_init_threads (void);
 SCM_INTERNAL void scm_init_thread_procs (void);
 SCM_INTERNAL void scm_init_threads_default_dynamic_state (void);
 
-- 
1.8.4