process: block SIGCHLD signal when spawning process

[sheremetyev]

Motivation

I'm observing that waiting for tokio::process::Command occasionally hangs on macOS (reproduced in Jujutsu version control CLI executed from inside jj_tui). Issue #6770 looks similar.

Solution

Inspecting the code, there seems to be a race possible between registering signal handler for SIGCHLD and receiving signal. Standard solution is to block signal during forking: https://stackoverflow.com/a/6757240. This is implemented in the pull request.

With this change I cannot reproduce hanging anymore.

[Darksonn]

@ipetkov what do you think?

At the very least, this code looks like it would break if called in parallel from two threads at once.

[notgull]

Wouldn't this cause child processes that complete while SIGCHLD is blocked to hang?

[ipetkov]

At the very least, this code looks like it would break if called in parallel from two threads at once.

Very much agree with this statement; this proposed change does not look safe in the general sense. Even if we were to introduce our own mutex here there's no guarantee that some other part of the process isn't trying to mess with signals.

---

I have two main questions:

1. Are we sure this race is the real culprit or just suspected to impact things? Hard to tell without any real steps to reproduce (though maybe the "fix" changes the execution order enough to not fall afoul of some other race condition). It's worth noting that the current implementation tries to be resilient against missed signals since we'll effectively always poll the child at least once to see if it has exited on start up.
2. If there really is a race between the child exiting before we've set up our signal handler, why do we need to block and unblock those signals instead of moving this line up to before spawning the child? This might be a much safer (and atomic) way of avoiding the race without introducing other races

[sheremetyev]

Thank you for reviewing! Here is a bit more context in case it's useful.

Steps to reproduce the issue:

• install Jujutsu (https://github.com/martinvonz/jj) version 0.22 or 0.23, or compile from master
• install jj_tui (https://github.com/faldor20/jj_tui/releases/tag/v0.8.8)
• create repository with jj git init and a few commits with jj commit
• run jj_tui and keep pressing key "e" to edit commit - it should hang after a few attempts (stops reacting to any key)
• ps aux | grep jj would show process jj edit ...
• running jj st or some other Jujutsu command in another terminal hangs (because it's waiting for file lock)
• attaching debugger to jj process would should it's waiting in mio::poll

(sorry I don't have a shorter way to reproduce!)

In order to test the fix in this PR, compile Jujutsu against custom version of Tokio

---

Are we sure this race is the real culprit or just suspected to impact things?

Not 100% sure.

...we'll effectively always poll the child at least once..

I observed that the ChildDropGuard: :poll function was called only 3 times in the hanging process. Then it was waiting in mio::poll - not entirely sure what's happening. Probably waiting for a signal wake up and run ChildDropGuard: :poll again?

---

... moving this line up to before spawning the child?

I did try installing signal before the spawn call - didn't help. IIRC installing signal handler is not guaranteed to have immediate effect (cannot find a reference at the moment).

---

Wouldn't this cause child processes that complete while SIGCHLD is blocked to hang?

Pending signals are delivered after unblocking (https://man7.org/linux/man-pages/man7/signal.7.html).

---

At the very least, this code looks like it would break if called in parallel from two threads at once.

Signal is blocked for all threads but unblocking it by one thread could affect concurrent calls to spawn_child indeed. I wonder if the current implementation installing signal handler safe for multithreading environments?

Playing with signals in a library is inherently fragile - it's unfortunate that macOS version is relying on signals IMHO. Linux implementation is using file descriptors.

[ipetkov]

I'd recommend running the test scenario under strace (or whatever is the equivalent on macOS). That should paint a clearer picture of what is happening, both from tokio's side as well as the rest of the application