Re: [PATCH 5/5] run-command: add note about forking and threading
- Date: Tue, 11 Apr 2017 00:53:48 +0000
- From: Eric Wong <e@xxxxxxxxx>
- Subject: Re: [PATCH 5/5] run-command: add note about forking and threading
Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Brandon Williams wrote:
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -458,6 +458,14 @@ int start_command(struct child_process *cmd)
> > argv_array_pushv(&argv, cmd->argv);
> > }
> > + /*
> > + * NOTE: In order to prevent deadlocking when using threads special
> > + * care should be taken with the function calls made in between the
> > + * fork() and exec() calls. No calls should be made to functions which
> > + * require acquiring a lock (e.g. malloc) as the lock could have been
> > + * held by another thread at the time of forking, causing the lock to
> > + * never be released in the child process.
> > + */
> > cmd->pid = fork();
> Why can't git use e.g. posix_spawn to avoid this?
posix_spawn does not support chdir, and it seems we run non-git
commands so no using "git -C" for those.
> fork()-ing in a threaded context is very painful for maintainability.
> Any library function you are using could start taking a lock, and then
> you have a deadlock. So you have to make use of a very small
> whitelisted list of library functions for this to work.
On the other hand, I believe we should make run-command
vfork-compatible (and Brandon's series is a big (but incomplete)
step in the (IMHO) right direction); as anything which is
vfork-safe would also be safe in the presence of threads+(plain) fork.
With vfork; the two processes share heap until execve.
I posted some notes about it last year:
> The function calls you have to audit are not only between fork() and
> exec() in the normal control flow. You have to worry about signal
> handlers, too.
Yes, all that auditing is necessary for vfork; too, but totally
doable. The mainline Ruby implementation has been using vfork
for spawning subprocesses for several years, now; and I think the
ruby-core developers (myself included) have fixed all the
problems with it; even in multi-threaded code which calls malloc.