[email protected] (Ludovic Courtès) writes: > [email protected] (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 <[email protected]> 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
