Re: [PATCH] Make guardians and (ice-9 popen) thread-safe

[ludo@gnu.org (Ludovic Courtès)]

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 <mhw@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 <mhw@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 <mhw@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 <mhw@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 <mhw@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 <mhw@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 <mhw@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’.