Hi! (I just realized when I finished my message that I was too late; sorry about that! Anyway, sending the few remaining questions I have.)
Mark H Weaver <m...@netris.org> skribis: > Here's a set of patches for stable-2.0 that make (ice-9 popen) and > guardians thread-safe. These patches fix <http://bugs.gnu.org/15683>. > (please disregard the preliminary patches I posted in that bug report). > > These patches also lay the groundwork for conveniently blocking asyncs > while mutexes are held, to prevent the deadlocks that can arise when > asyncs are run while a mutex is held, and one of the asyncs runs code > that tries to lock the same mutex, as discussed here: > > http://lists.gnu.org/archive/html/guile-devel/2013-08/msg00025.html > http://lists.gnu.org/archive/html/guile-devel/2013-08/msg00030.html > > In this patch set, I only block asyncs in two places: when the guardian > mutex is held, and when the 'overrides_lock' is held in procprop.scm, > which has caused deadlocks for me in practice. However, in the future > I'd like to make a more comprehensive effort to find other places where > this should be done. As already discussed, this looks like the Right Thing to me. > From e4b90f498c3141ba203a6359a8a0cfe790324203 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver <m...@netris.org> > Date: Sun, 17 Nov 2013 04:00:29 -0500 > Subject: [PATCH 1/6] Add mutex locking functions that also block asyncs. > > * libguile/async.h (scm_i_pthread_mutex_lock_with_asyncs, > scm_i_pthread_mutex_unlock_with_asyncs): New macros. > > * libguile/threads.c (do_unlock_with_asyncs): New static helper. > (scm_i_dynwind_pthread_mutex_lock_with_asyncs): New function. > > * libguile/threads.h (scm_i_dynwind_pthread_mutex_lock_with_asyncs): > Add prototype. OK, but: > +# define scm_i_pthread_mutex_lock_with_asyncs(m) \ > + do \ > + { \ > + SCM_I_CURRENT_THREAD->block_asyncs++; \ > + scm_i_pthread_mutex_lock (m); \ > + } \ > + while (0) I find the name possibly confusing. What about ‘scm_i_pthread_mutex_lock_block_asyncs’ and ‘scm_i_pthread_mutex_unlock_unlock_asyncs’? > +static void > +do_unlock_with_asyncs (void *data) > +{ > + scm_i_pthread_mutex_unlock ((scm_i_pthread_mutex_t *)data); > + SCM_I_CURRENT_THREAD->block_asyncs--; > +} > + > +void > +scm_i_dynwind_pthread_mutex_lock_with_asyncs (scm_i_pthread_mutex_t *mutex) Likewise. > From 13fcd175bc31b5df400dd518836bdce32f32206a Mon Sep 17 00:00:00 2001 > From: Mark H Weaver <m...@netris.org> > Date: Sun, 17 Nov 2013 03:19:32 -0500 > Subject: [PATCH 2/6] Block system asyncs while 'overrides_lock' is held. > > * libguile/procprop.c (scm_set_procedure_property_x): Block system > asyncs while overrides_lock is held. Use dynwind block in case > an exception is thrown. [...] > - scm_i_pthread_mutex_lock (&overrides_lock); > + scm_dynwind_begin (0); > + scm_i_dynwind_pthread_mutex_lock_with_asyncs (&overrides_lock); > props = scm_hashq_ref (overrides, proc, SCM_BOOL_F); Could you recap why asyncs must be blocked here, and add it as a comment? I would think that there’s no way ‘scm_hashq_ref’ can trigger an async run, because it doesn’t call out to Scheme, does it? > From 73d674d1983e7777f22d43335a1834bf26606494 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver <m...@netris.org> > Date: Sun, 17 Nov 2013 03:35:09 -0500 > Subject: [PATCH 3/6] Make guardians thread-safe. > > * libguile/guardians.c (t_guardian): Add mutex. > (finalize_guarded, scm_i_guard, scm_i_get_one_zombie): Lock mutex and > block system asyncs during critical sections. > (scm_make_guardian): Initialize mutex. OK. > From f89b0a7adf9260fee39dd82e756edfabcdf1a668 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver <m...@netris.org> > Date: Sun, 17 Nov 2013 01:11:57 -0500 > Subject: [PATCH 4/6] Make port alists accessible from Scheme. > > * libguile/ports.c (scm_i_port_alist, scm_i_set_port_alist_x): Make > these available from Scheme, as '%port-alist' and '%set-port-alist!'. > Validate port argument. > > * libguile/ports.h (scm_i_set_port_alist_x): Change return type from > 'void' to 'SCM'. OK, but perhaps ‘%port-properties’ and ‘scm_i_port_properties’? (I know the field has been called ‘alist’ from some time, but now that it is somewhat visible from Scheme, names matter more IMO.) Also, I wonder if this should rather be exposed as ‘%port-property KEY’ and ‘%set-port-property! PORT KEY VALUE’. Those two procedures could eventually do any locking required. > From b53342d28a0ec0844373c1469d5f56d4cb6d98fc Mon Sep 17 00:00:00 2001 > From: Mark H Weaver <m...@netris.org> > Date: Sun, 17 Nov 2013 02:46:08 -0500 > Subject: [PATCH 5/6] Stylistic improvements for (ice-9 popen). > > * module/ice-9/popen.scm (close-process, close-process-quietly): Accept > 'port' and 'pid' as separate arguments. Improve style. > (close-pipe, read-pipes): Improve style. OK. > From 3c3e66b85d294fc5ded6f6660a0efe00ed6519b9 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver <m...@netris.org> > Date: Sun, 17 Nov 2013 02:54:31 -0500 > Subject: [PATCH 6/6] Make (ice-9 popen) thread-safe. > > * module/ice-9/popen.scm: Import (ice-9 threads). > (port/pid-table): Mark as deprecated in comment. > (port/pid-table-mutex): New variable. > (open-pipe*): Store the pid in the port's alist. Guard the alist > entry instead of the port. Lock 'port/pid-table-mutex' while mutating > 'port/pid-table'. > (fetch-pid): Removed. > (fetch-alist-entry): New procedure. > (close-process-quietly): Removed. > (close-pipe): Use 'fetch-alist-entry' instead of 'fetch-pid'. Clear > the cdr of the alist entry. Improve error messages. > (reap-pipes): Adapt to the fact that the alist entries are now guarded > instead of the ports. Incorporate the 'waitpid' code that was > previously in 'close-process-quietly', but let the port finalizer > close the port. Clear the cdr of the alist entry. OK. Thanks! Ludo’.
