Does Guile have a thread limit?

Does Guile have a thread limit?

[Diogo F. S. Ramos]

The following program is aborted:

--8<---------------cut here---------------start------------->8---
(define number-of-thread 1000)

(do ((i number-of-thread (- i 1)))
    ((zero? i))
  (call-with-new-thread (lambda () (sleep 42))))
--8<---------------cut here---------------end--------------->8---

Re: Does Guile have a thread limit?

[Eli Zaretskii]

> From: "Diogo F. S. Ramos" <dfsr@riseup.net>
> Date: Sat, 05 Apr 2014 03:28:25 -0300
> 
> The following program is aborted:
> 
> --8<---------------cut here---------------start------------->8---
> (define number-of-thread 1000)
> 
> (do ((i number-of-thread (- i 1)))
>     ((zero? i))
>   (call-with-new-thread (lambda () (sleep 42))))
> --8<---------------cut here---------------end--------------->8---

On what OS?

Re: Does Guile have a thread limit?

[Taylan Ulrich Bayırlı/Kammer]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: "Diogo F. S. Ramos" <dfsr@riseup.net>
>> Date: Sat, 05 Apr 2014 03:28:25 -0300
>> 
>> The following program is aborted:
>> 
>> --8<---------------cut here---------------start------------->8---
>> (define number-of-thread 1000)
>> 
>> (do ((i number-of-thread (- i 1)))
>>     ((zero? i))
>>   (call-with-new-thread (lambda () (sleep 42))))
>> --8<---------------cut here---------------end--------------->8---
>
> On what OS?

I can reproduce on OpenBSD 5.3 with Guile 2.0.11.

I wasn't sure if OpenBSD perhaps limits the number of pthreads allowed
to a process, so I used the following C program to test this, which had
all 1000 threads succeed:

/* start C code */

#include <pthread.h>
#include <stdio.h>
#include <stddef.h>

static void *thread_entry(void *arg)
{
  printf("thread %d succeeded\n", (ptrdiff_t)arg);
  return NULL;
}

int main()
{
  ptrdiff_t i;
  for (i = 0; i < 1000; ++i)
    {
      pthread_t thr;
      pthread_create(&thr, NULL, thread_entry, (void *)i);
    }
  sleep(5);
  puts("main finished");
  return 0;
}

/* end C code */

Taylan

Re: Does Guile have a thread limit?

[Diogo F. S. Ramos]

>> From: "Diogo F. S. Ramos" <dfsr@riseup.net>
>> Date: Sat, 05 Apr 2014 03:28:25 -0300
>> 
>> The following program is aborted:
>> 
>> --8<---------------cut here---------------start------------->8---
>> (define number-of-thread 1000)
>> 
>> (do ((i number-of-thread (- i 1)))
>>     ((zero? i))
>>   (call-with-new-thread (lambda () (sleep 42))))
>> --8<---------------cut here---------------end--------------->8---
>
> On what OS?

Debian Sid, Guile 2.0.11.

Re: Does Guile have a thread limit?

[Mark H Weaver]

"Diogo F. S. Ramos" <dfsr@riseup.net> writes:

> The following program is aborted:
>
> (define number-of-thread 1000)
>
> (do ((i number-of-thread (- i 1)))
>     ((zero? i))
>   (call-with-new-thread (lambda () (sleep 42))))

I looked into this, and the issue is that for every thread that's ever
put into Guile mode, Guile creates a pipe which is used to wake it up
while it's sleeping or waiting in 'select', e.g. if an async is queued.

Therefore, two file descriptors are allocated for each such thread.

Unfortunately, if you run out of file descriptors while creating a
thread, it simply aborts, which is obviously suboptimal.  The relevant
code is in threads.c:563.

    Mark

Re: Does Guile have a thread limit?

[Taylan Ulrich Bayırlı/Kammer]

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

Mark H Weaver <mhw@netris.org> writes:

> "Diogo F. S. Ramos" <dfsr@riseup.net> writes:
>
>> The following program is aborted:
>>
>> (define number-of-thread 1000)
>>
>> (do ((i number-of-thread (- i 1)))
>>     ((zero? i))
>>   (call-with-new-thread (lambda () (sleep 42))))
>
> I looked into this, and the issue is that for every thread that's ever
> put into Guile mode, Guile creates a pipe which is used to wake it up
> while it's sleeping or waiting in 'select', e.g. if an async is queued.
>
> Therefore, two file descriptors are allocated for each such thread.
>
> Unfortunately, if you run out of file descriptors while creating a
> thread, it simply aborts, which is obviously suboptimal.  The relevant
> code is in threads.c:563.
>
>     Mark

Here's a patch to fix this.  I'm a novice in C projects and the Guile
code-base so please criticize as much as possible!

I have an equivalent patch for stable-2.0, except that it doesn't add
the `int *err' argument to `scm_with_guile'.  (The void to int return
type changes in some other public functions is kept, because I imagine
it's unlikely that existing code *relies* on void returns.)

However, strangely, when I *did* also add the `int *err' argument to
`scm_with_guile' on stable-2.0, I would get occasional segfaults during
`make check', which I can't make sense of.  Example backtrace from a
core dump: http://sprunge.us/LhSR .  BTW this is OpenBSD 5.3.  Does
anyone have ideas?

I can't be sure whether this patch for master perhaps suffers from the
same issue (since it does add the extra argument), because I already get
`make check' failures as it stands, though my patch doesn't seem to
affect where I get them (using make -k).

I'm leaving the patch for stable-2.0 for later, since it's based on this
one and any suggested changes will be carried over:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Handle thread creation failure --]
[-- Type: text/x-patch, Size: 17090 bytes --]

From dd5eb7e1d85d3e23e58f027f4b427752a32332fa Mon Sep 17 00:00:00 2001
From: Taylan Ulrich B <taylanbayirli@gmail.com>
Date: Tue, 8 Apr 2014 21:33:43 +0200
Subject: [PATCH] Handle thread creation failure

* libguile/init.h (scm_init_guile): Return int for errors.
* libguile/threads.h (scm_with_guile): Take `int *err' argument.
(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.
(do_thread_exit_trampoline): Pass third arg to `scm_with_guile'.
(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 init failure.
(scm_i_with_guile_and_parent): Handle `with_guile_and_parent' error.
(scm_with_guile): Take `int *err' argument.
(launch_data): Add `int err' field.
(launch_thread): Handle `scm_i_with_guile_and_parent' error.
(call-with-new-thread): Signal thread creation failure.
(spawn_data): Add `int err' field.
(spawn_thread): Handle `scm_i_with_guile_and_parent' error.
(scm_threads_prehistory): Return int for errors.
(scm_init_threads): Return int for errors.

* libguile/init.c:
* libguile/finalizers.c:
* test-suite/standalone/test-pthread-create-secondary.c:
* test-suite/standalone/test-pthread-create.c:
* test-suite/standalone/test-scm-c-read.c:
* test-suite/standalone/test-scm-spawn-thread.c:
* test-suite/standalone/test-scm-take-locale-symbol.c:
* test-suite/standalone/test-scm-take-u8vector.c:
* test-suite/standalone/test-scm-with-guile.c:
* test-suite/standalone/test-with-guile-module.c: Pass NULL to
  `scm_with_guile' for the int *err argument.
---
 libguile/finalizers.c                              |  2 +-
 libguile/init.c                                    |  4 +-
 libguile/init.h                                    |  2 +-
 libguile/threads.c                                 | 93 ++++++++++++++++------
 libguile/threads.h                                 |  6 +-
 .../standalone/test-pthread-create-secondary.c     |  2 +-
 test-suite/standalone/test-pthread-create.c        |  4 +-
 test-suite/standalone/test-scm-c-read.c            |  2 +-
 test-suite/standalone/test-scm-spawn-thread.c      |  2 +-
 .../standalone/test-scm-take-locale-symbol.c       |  2 +-
 test-suite/standalone/test-scm-take-u8vector.c     |  2 +-
 test-suite/standalone/test-scm-with-guile.c        |  4 +-
 test-suite/standalone/test-with-guile-module.c     |  4 +-
 13 files changed, 85 insertions(+), 44 deletions(-)

diff --git a/libguile/finalizers.c b/libguile/finalizers.c
index eaea139..b8cf854 100644
--- a/libguile/finalizers.c
+++ b/libguile/finalizers.c
@@ -239,7 +239,7 @@ finalization_thread_proc (void *unused)
 static void*
 run_finalization_thread (void *arg)
 {
-  return scm_with_guile (finalization_thread_proc, arg);
+  return scm_with_guile (finalization_thread_proc, arg, NULL);
 }
 
 static void
diff --git a/libguile/init.c b/libguile/init.c
index 81cf997..313e1be 100644
--- a/libguile/init.c
+++ b/libguile/init.c
@@ -315,7 +315,7 @@ scm_boot_guile (int argc, char ** argv, void (*main_func) (), void *closure)
   c.argc = argc;
   c.argv = argv;
 
-  res = scm_with_guile (invoke_main_func, &c);
+  res = scm_with_guile (invoke_main_func, &c, NULL);
 
   /* If the caller doesn't want this, they should exit from main_func
      themselves.
@@ -371,7 +371,7 @@ cleanup_for_exit ()
   /* This function might be called in non-guile mode, so we need to
      enter it temporarily. 
   */
-  scm_with_guile (really_cleanup_for_exit, NULL);
+  scm_with_guile (really_cleanup_for_exit, NULL, NULL);
 }
 
 void
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..532270f 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;
 }
 
 \f
@@ -597,7 +598,7 @@ do_thread_exit_trampoline (struct GC_stack_base *sb, void *v)
   GC_register_my_thread (sb);
 #endif
 
-  return scm_with_guile (do_thread_exit, v);
+  return scm_with_guile (do_thread_exit, v, NULL);
 }
 
 static void
@@ -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,29 @@ 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 *ret;
 
   args.func = func;
   args.data = data;
   args.parent = parent;
-  
-  return GC_call_with_stack_base (with_guile_and_parent, &args);
+  args.err = 0;
+
+  ret = GC_call_with_stack_base (with_guile_and_parent, &args);
+  if (err)
+    *err = args.err;
+  return ret;
 }
 
 void *
-scm_with_guile (void *(*func)(void *), void *data)
+scm_with_guile (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 +883,7 @@ typedef struct {
   SCM thread;
   scm_i_pthread_mutex_t mutex;
   scm_i_pthread_cond_t cond;
+  int err;
 } launch_data;
 
 static void *
@@ -896,7 +913,13 @@ launch_thread (void *d)
 {
   launch_data *data = (launch_data *)d;
   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, &data->err);
+  if (data->err)
+    {
+      scm_i_scm_pthread_mutex_lock (&data->mutex);
+      scm_i_pthread_cond_signal (&data->cond);
+      scm_i_pthread_mutex_unlock (&data->mutex);
+    }
   return NULL;
 }
 
@@ -929,6 +952,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 +963,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 launch thread", SCM_EOL);
+
   return data.thread;
 }
 #undef FUNC_NAME
@@ -957,6 +984,7 @@ typedef struct {
   SCM thread;
   scm_i_pthread_mutex_t mutex;
   scm_i_pthread_cond_t cond;
+  int err;
 } spawn_data;
 
 static void *
@@ -989,7 +1017,13 @@ spawn_thread (void *d)
 {
   spawn_data *data = (spawn_data *)d;
   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, &data->err);
+  if (data->err)
+    {
+      scm_i_scm_pthread_mutex_lock (&data->mutex);
+      scm_i_pthread_cond_signal (&data->cond);
+      scm_i_pthread_mutex_unlock (&data->mutex);
+    }
   return NULL;
 }
 
@@ -1009,6 +1043,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 +1054,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 +2108,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 +2127,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 +2148,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..62db18a 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -136,11 +136,11 @@ SCM_API SCM scm_spawn_thread (scm_t_catch_body body, void *body_data,
 			      scm_t_catch_handler handler, void *handler_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 (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);
 
diff --git a/test-suite/standalone/test-pthread-create-secondary.c b/test-suite/standalone/test-pthread-create-secondary.c
index 14ea240..71f8f82 100644
--- a/test-suite/standalone/test-pthread-create-secondary.c
+++ b/test-suite/standalone/test-pthread-create-secondary.c
@@ -56,7 +56,7 @@ do_something (void *arg)
 static void *
 thread (void *arg)
 {
-  scm_with_guile (do_something, NULL);
+  scm_with_guile (do_something, NULL, NULL);
   return NULL;
 }
 
diff --git a/test-suite/standalone/test-pthread-create.c b/test-suite/standalone/test-pthread-create.c
index cf3771f..ef3e80b 100644
--- a/test-suite/standalone/test-pthread-create.c
+++ b/test-suite/standalone/test-pthread-create.c
@@ -38,7 +38,7 @@ do_something (void *arg)
 static void *
 thread (void *arg)
 {
-  scm_with_guile (do_something, NULL);
+  scm_with_guile (do_something, NULL, NULL);
   return NULL;
 }
 
@@ -63,7 +63,7 @@ inner_main (void *data)
 int
 main (int argc, char *argv[])
 {
-  scm_with_guile (inner_main, NULL);
+  scm_with_guile (inner_main, NULL, NULL);
 
   return EXIT_SUCCESS;
 }
diff --git a/test-suite/standalone/test-scm-c-read.c b/test-suite/standalone/test-scm-c-read.c
index 4111cd0..745ed86 100644
--- a/test-suite/standalone/test-scm-c-read.c
+++ b/test-suite/standalone/test-scm-c-read.c
@@ -125,7 +125,7 @@ do_start (void *arg)
 int
 main (int argc, char *argv[])
 {
-  scm_with_guile (do_start, NULL);
+  scm_with_guile (do_start, NULL, NULL);
 
   return 0;
 }
diff --git a/test-suite/standalone/test-scm-spawn-thread.c b/test-suite/standalone/test-scm-spawn-thread.c
index f6d561a..51ea164 100644
--- a/test-suite/standalone/test-scm-spawn-thread.c
+++ b/test-suite/standalone/test-scm-spawn-thread.c
@@ -57,6 +57,6 @@ main (int argc, char **argv)
 {
   SCM result;
 
-  result = PTR2SCM (scm_with_guile (inner_main, 0));
+  result = PTR2SCM (scm_with_guile (inner_main, 0, NULL));
   return scm_is_true (result) ? EXIT_SUCCESS : EXIT_FAILURE;
 }
diff --git a/test-suite/standalone/test-scm-take-locale-symbol.c b/test-suite/standalone/test-scm-take-locale-symbol.c
index 808068f..379e180 100644
--- a/test-suite/standalone/test-scm-take-locale-symbol.c
+++ b/test-suite/standalone/test-scm-take-locale-symbol.c
@@ -58,7 +58,7 @@ main (int argc, char *argv[])
 {
   int result;
 
-  scm_with_guile (do_test, &result);
+  scm_with_guile (do_test, &result, NULL);
 
   return result;
 }
diff --git a/test-suite/standalone/test-scm-take-u8vector.c b/test-suite/standalone/test-scm-take-u8vector.c
index fff3af4..1b2211e 100644
--- a/test-suite/standalone/test-scm-take-u8vector.c
+++ b/test-suite/standalone/test-scm-take-u8vector.c
@@ -59,7 +59,7 @@ main (int argc, char *argv[])
 {
   int result;
 
-  scm_with_guile (do_test, &result);
+  scm_with_guile (do_test, &result, NULL);
 
   return result;
 }
diff --git a/test-suite/standalone/test-scm-with-guile.c b/test-suite/standalone/test-scm-with-guile.c
index a78458e..7612a2f 100644
--- a/test-suite/standalone/test-scm-with-guile.c
+++ b/test-suite/standalone/test-scm-with-guile.c
@@ -49,7 +49,7 @@ go_deeper_into_the_stack (unsigned level)
   if (level > 0)
     go_deeper_into_the_stack (level - 1);
   else
-    scm_with_guile (entry_point, NULL);
+    scm_with_guile (entry_point, NULL, NULL);
 }
 
 \f
@@ -61,7 +61,7 @@ main (int argc, char *argv[])
 
   /* Invoke it from much higher into the stack.  This time, Guile is expected
      to update the `base' field of the current thread.  */
-  scm_with_guile (entry_point, NULL);
+  scm_with_guile (entry_point, NULL, NULL);
 
   return 0;
 }
diff --git a/test-suite/standalone/test-with-guile-module.c b/test-suite/standalone/test-with-guile-module.c
index 4e22ff5..9110827 100644
--- a/test-suite/standalone/test-with-guile-module.c
+++ b/test-suite/standalone/test-with-guile-module.c
@@ -47,7 +47,7 @@ thread_inner_main (void *unused)
 void *
 thread_main (void *unused)
 {
-  scm_with_guile (&thread_inner_main, NULL);
+  scm_with_guile (&thread_inner_main, NULL, NULL);
 
   return NULL;			/* dummy */
 }
@@ -76,7 +76,7 @@ inner_main (void *unused)
 int
 main (int argc, char **argv)
 {
-  scm_with_guile (&inner_main, NULL);
+  scm_with_guile (&inner_main, NULL, NULL);
 
   return 0;
 }
-- 
1.8.1.2

Re: Does Guile have a thread limit?

[Ludovic Courtès]

Hi,

Thanks for looking into it!

However, I see two issues:

  1. We can’t really change ‘scm_with_guile’, because it’s a public
     function, and it’s been there “forever”.

  2. Fundamentally, there should be no limit other than that imposed by
     the OS (or hardware resources) on the number of threads.

Doug Evans (hi! :-)) was looking at signal-handling code, suggesting he
may come up with another implementation of signal-handling that does not
involve the current signal delivery machinery.

That machinery happens to also be the root of the problem at hand,
because it mandates this pipe between newly-created threads and the
signal-delivery thread.

So I think this is really the way to go.

Ludo’.

Re: Does Guile have a thread limit?

[Taylan Ulrich Bayırlı/Kammer]

ludo@gnu.org (Ludovic Courtès) writes:

> Hi,
>
> Thanks for looking into it!
>
> However, I see two issues:
>
>   1. We can’t really change ‘scm_with_guile’, because it’s a public
>      function, and it’s been there “forever”.

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?)

>   2. Fundamentally, there should be no limit other than that imposed by
>      the OS (or hardware resources) on the number of threads.
>
> Doug Evans (hi! :-)) was looking at signal-handling code, suggesting he
> may come up with another implementation of signal-handling that does not
> involve the current signal delivery machinery.
>
> That machinery happens to also be the root of the problem at hand,
> because it mandates this pipe between newly-created threads and the
> signal-delivery thread.
>
> So I think this is really the way to go.

After that change is done, will we be able to guarantee that thread
initialization is *always* successful?  I see that the return value of
pthread_create() is already handled, so if we can guarantee that
everything happening after it will always succeed in initializing the
created thread, then this propagation of errors is indeed redundant.

Taylan

Re: Does Guile have a thread limit?

[Ludovic Courtès]

taylanbayirli@gmail.com (Taylan Ulrich "Bayırlı/Kammer") skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Hi,
>>
>> Thanks for looking into it!
>>
>> However, I see two issues:
>>
>>   1. We can’t really change ‘scm_with_guile’, because it’s a public
>>      function, and it’s been there “forever”.
>
> 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.

>>   2. Fundamentally, there should be no limit other than that imposed by
>>      the OS (or hardware resources) on the number of threads.
>>
>> Doug Evans (hi! :-)) was looking at signal-handling code, suggesting he
>> may come up with another implementation of signal-handling that does not
>> involve the current signal delivery machinery.
>>
>> That machinery happens to also be the root of the problem at hand,
>> because it mandates this pipe between newly-created threads and the
>> signal-delivery thread.
>>
>> So I think this is really the way to go.
>
> 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’.

Re: Does Guile have a thread limit?

[Taylan Ulrich Bayırlı/Kammer]

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

ludo@gnu.org (Ludovic Courtès) writes:

> taylanbayirli@gmail.com (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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Handle thread initialization failure --]
[-- Type: text/x-patch, Size: 10733 bytes --]

From 37cbfdedfc0523f52f7d45f737998a4a45ff9ffa Mon Sep 17 00:00:00 2001
From: Taylan Ulrich B <taylanbayirli@gmail.com>
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;
 }
 
 \f
@@ -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

Re: Does Guile have a thread limit?

[Taylan Ulrich Bayirli/Kammer]

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

taylanbayirli@gmail.com (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.)


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

From c4ff5443b85e9840b35b0d0a7e8a70cbbba35f25 Mon Sep 17 00:00:00 2001
From: Taylan Ulrich B <taylanbayirli@gmail.com>
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;
 }
 
 \f
@@ -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

Re: Does Guile have a thread limit?

[Mark H Weaver]

Hi,

Taylan Ulrich Bayirli/Kammer <taylanbayirli@gmail.com> writes:

> taylanbayirli@gmail.com (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.

One serious problem with this patch is that it changes the API (and ABI)
of 'scm_init_guile'.  This function has never returned an error code and
thus has always been assumed to succeed.  This patch would allow it to
fail and return an error code that no one ever checks.  There are other
problems with the patch, but I'm not sure it's worth enumerating them
because I think this is the wrong approach.

The fundamental problem here is that currently, programs must be careful
to limit the number of threads that have ever been put into Guile mode,
just as they must limit the number of open files.  If they do that, then
they won't ever encounter this error.  If they run out of FDs then they
are screwed, and this patch doesn't change that fact, it merely trades
one kind of ungraceful exit for another.

A proper solution would eliminate that limitation.  Once we've done
that, then there will no longer be a need for these new APIs that you
would introduce, nor for the error-prone code paths that they would
entail.

> (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.)

I don't think it's a different issue.  Rather, I think it demonstrates
that this patch doesn't really solve the problem in practice.

      Mark

Re: Does Guile have a thread limit?

[Taylan Ulrich Bayirli/Kammer]

Mark H Weaver <mhw@netris.org> writes:

> One serious problem with this patch is that it changes the API (and
> ABI) of 'scm_init_guile'.

The public functions could be separated into a compatible and a _safe
variant like I did with `scm_with_guile', and `scm_with_guile_safe',
should the patch be otherwise accepted.

(I thought it would be fine to change a void return type to another, but
I guess that was ignorance on my side.)

> The fundamental problem here is that currently, programs must be
> careful to limit the number of threads that have ever been put into
> Guile mode, just as they must limit the number of open files.  If they
> do that, then they won't ever encounter this error.  If they run out
> of FDs then they are screwed, and this patch doesn't change that fact,
> it merely trades one kind of ungraceful exit for another.
>
> A proper solution would eliminate that limitation.  Once we've done
> that, then there will no longer be a need for these new APIs that you
> would introduce, nor for the error-prone code paths that they would
> entail.

I think there are two things to ask here:

1. once the FD limitation is lifted, can we otherwise guarantee that
   thread initialization will always succeed (after pthread_create), and
   do we want to hold on to that guarantee for the future (not ever
   introduce possibly-failing code to thread initialization)?

And if we *don't* want to make that guarantee,

2. should we support error-reporting variants of the public functions at
   the expense of code complexity?

I thought the right answer was 1. we can't guarantee that and 2. we
should at least provide a way to handle errors, but if you say otherwise
I'm fine with that; just asking explicitly to make the situation clear.
(And we might want to document the conclusion.)

(A more over-arching question might be whether it's acceptable for Guile
to abort or crash at all, or in which precise situations.  That would be
relevant to the IO thread-safety topic as well.  A *fully* safe and
fault-tolerant system is a nice thought, but maybe an impractical goal.)

Taylan