guile-user
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Does Guile have a thread limit?


From: Taylan Ulrich Bayırlı /Kammer
Subject: Re: Does Guile have a thread limit?
Date: Thu, 10 Apr 2014 23:22:14 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (berkeley-unix)

address@hidden (Ludovic Courtès) writes:

> address@hidden (Taylan Ulrich "Bayırlı/Kammer") skribis:
>
>> OK, I expected this.  We could just make that one abort then, and
>> keep the exception in `call-with-new-thread' and similar.  (Or are
>> there other plausible ways to signal an error in C, other than return
>> value and error-pointer argument?)
>
> No, that’s a problem.
>
>> After that change is done, will we be able to guarantee that thread
>> initialization is *always* successful?
>
> No, we can never guarantee that.
>
> We could introduce a variant of scm_with_guile2, but I’m not sure
> there’s anything sensible that can be done anyway.
>
> What matters, though, is for Scheme-level procedures that create
> threads to not abort.
>
> Ludo’.

Here's an improved patch.  Other from the minor issue of using the
phrase "thread initialization" more consistently (in place of "creation"
or "launching"), it has the following differences:

- scm_with_guile() doesn't take an `int *err' argument, it aborts on
  error.  While an abort is nasty, the only alternative I see is a
  silent failure, which I think is worse; correct me if I'm wrong.

- There is scm_with_guile_safe() which takes an `int *err'.

- The `int err' fields of launch_data and spawn_data structs aren't
  written to without locking the mutex (since they're being read from
  the other thread); this might have been the reason of some segfaults I
  got (though I can't really imagine how, because it should only happen
  when thread initialization *does* fail, which isn't triggered by
  anything in the test-suite).

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

>From 37cbfdedfc0523f52f7d45f737998a4a45ff9ffa 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 | 108 +++++++++++++++++++++++++++++++++++++++++------------
 libguile/threads.h |   5 ++-
 3 files changed, 88 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 dd04f6f..754d517 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -394,7 +394,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;
@@ -434,10 +434,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;
@@ -467,11 +464,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;
@@ -503,6 +502,8 @@ guilify_self_2 (SCM parent)
 
   /* See note in finalizers.c:queue_finalizer_async().  */
   GC_invoke_finalizers ();
+
+  return 0;
 }
 
 
@@ -683,7 +684,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
@@ -733,21 +734,23 @@ 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)
+            return -1;
+          if (guilify_self_2 (parent) != 0)
+            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");
@@ -760,6 +763,7 @@ struct with_guile_args
   GC_fn_type func;
   void *data;
   SCM parent;
+  int err;
 };
 
 static void *
@@ -779,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)
     {
@@ -820,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 *
@@ -867,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 *
@@ -895,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;
 }
 
@@ -929,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);
@@ -939,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
@@ -957,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 *
@@ -988,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;
 }
 
@@ -1009,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);
@@ -1019,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;
@@ -2070,7 +2127,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
@@ -2089,14 +2146,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));
@@ -2110,10 +2167,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.1.2


reply via email to

[Prev in Thread] Current Thread [Next in Thread]