job-control: do system() and :! in Nvim's process-group

(This commit is for reference; the functional change will be reverted.)

ref neovim#8217
ref neovim#8450
ref neovim#8678

In terminal-Vim, system() and :! run in Vim's process-group. But
8d90171 changed all of Nvim's process-spawn utilities to do
setsid(), which conflicts with that expected terminal-Vim behavior.

To "fix" that, this commit defines Process.detach as a TriState, then
handles the kNone case such that system() and :! do not do setsid() in
the spawned child.

But this commit REGRESSES 8d90171 (neovim#8107), so for example the
following code causes orphan processes:
    :echo system('sleep 30|sleep 30|sleep 30')

Q: If we don't create a new session/process-group, how to avoid zombie
   descendants (e.g. process_wait() calls process_stop(), which only
   kills the root process)?
A: Vim's approach in mch_call_shell_fork() is:
   1. BLOCK_SIGNALS (ignores deadly)
   2. fork()
   3. unblock signals in the child
   4. On CTRL-C, send SIGINT to the process-group id: kill(-pid, SIGINT)
   5. Parent (vim) ignores the signal. Child (and descendants) do not.
   https://github.com/vim/vim/blob/e7499ddc33508d3d341e96f84a0e7b95b2d6927c/src/os_unix.c#L4834-L4841
   https://github.com/vim/vim/blob/e7499ddc33508d3d341e96f84a0e7b95b2d6927c/src/os_unix.c#L5122-L5134

But we can't do that if we want to use the existing (libuv-based) form
of process_spawn().

src/nvim/channel.c

@@ -317,7 +317,7 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
   proc->argv = argv;
   proc->cb = channel_process_exit_cb;
   proc->events = chan->events;
-  proc->detach = detach;
+  proc->detach = detach ? kTrue : kFalse;
   proc->cwd = cwd;
 
   char *cmd = xstrdup(proc->argv[0]);

src/nvim/event/libuv_process.c

@@ -32,12 +32,15 @@ int libuv_process_spawn(LibuvProcess *uvproc)
   if (os_shell_is_cmdexe(proc->argv[0])) {
     uvproc->uvopts.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS;
   }
-  if (proc->detach) {
+  if (proc->detach == kTrue) {
     uvproc->uvopts.flags |= UV_PROCESS_DETACHED;
   }
 #else
-  // Always setsid() on unix-likes. #8107
-  uvproc->uvopts.flags |= UV_PROCESS_DETACHED;
+  // Always setsid() on unix. #8107
+  // Except for system(), systemlist(), :!. #8217
+  if (proc->detach != kNone) {
+    uvproc->uvopts.flags |= UV_PROCESS_DETACHED;
+  }
 #endif
   uvproc->uvopts.exit_cb = exit_cb;
   uvproc->uvopts.cwd = proc->cwd;

src/nvim/event/process.c

@@ -120,7 +120,7 @@ void process_teardown(Loop *loop) FUNC_ATTR_NONNULL_ALL
   process_is_tearing_down = true;
   kl_iter(WatcherPtr, loop->children, current) {
     Process *proc = (*current)->data;
-    if (proc->detach || proc->type == kProcessTypePty) {
+    if (proc->detach == kTrue || proc->type == kProcessTypePty) {
       // Close handles to process without killing it.
       CREATE_EVENT(loop->events, process_close_handles, 1, proc);
     } else {
@@ -144,9 +144,10 @@ void process_close_streams(Process *proc) FUNC_ATTR_NONNULL_ALL
 
 /// Synchronously wait for a process to finish
 ///
-/// @param process  Process instance
+/// @param proc     Process instance
 /// @param ms       Time in milliseconds to wait for the process.
 ///                 0 for no wait. -1 to wait until the process quits.
+/// @param events   Event-queue, or NULL to use `proc.events`.
 /// @return Exit code of the process. proc->status will have the same value.
 ///         -1 if the timeout expired while the process is still running.
 ///         -2 if the user interruped the wait.
@@ -212,20 +213,27 @@ void process_stop(Process *proc) FUNC_ATTR_NONNULL_ALL
   proc->stopped_time = os_hrtime();
 
   switch (proc->type) {
-    case kProcessTypeUv:
+    case kProcessTypeUv: {
       // Close the process's stdin. If the process doesn't close its own
       // stdout/stderr, they will be closed when it exits(possibly due to being
       // terminated after a timeout)
       stream_may_close(&proc->in);
-      os_proc_tree_kill(proc->pid, SIGTERM);
+      if (proc->detach != kNone) {
+        os_proc_tree_kill(proc->pid, SIGTERM);
+      } else {
+        uv_kill(proc->pid, SIGTERM);
+      }
       break;
-    case kProcessTypePty:
+    }
+    case kProcessTypePty: {
       // close all streams for pty processes to send SIGHUP to the process
       process_close_streams(proc);
       pty_process_close_master((PtyProcess *)proc);
       break;
-    default:
+    }
+    default: {
       abort();
+    }
   }
 
   // (Re)start timer to verify that stopped process(es) died.
@@ -292,16 +300,16 @@ static void decref(Process *proc)
 static void process_close(Process *proc)
   FUNC_ATTR_NONNULL_ARG(1)
 {
-  if (process_is_tearing_down && (proc->detach || proc->type == kProcessTypePty)
+  if (process_is_tearing_down
+      && (proc->detach == kTrue || proc->type == kProcessTypePty)
       && proc->closed) {
-    // If a detached/pty process dies while tearing down it might get closed
-    // twice.
+    // If a detached/pty process dies during teardown it might get closed twice.
     return;
   }
   assert(!proc->closed);
   proc->closed = true;
 
-  if (proc->detach) {
+  if (proc->detach == kTrue) {
     if (proc->type == kProcessTypeUv) {
       uv_unref((uv_handle_t *)&(((LibuvProcess *)proc)->uv));
     }

src/nvim/event/process.h

@@ -4,6 +4,7 @@
 #include "nvim/event/loop.h"
 #include "nvim/event/rstream.h"
 #include "nvim/event/wstream.h"
+#include "nvim/types.h"
 
 typedef enum {
   kProcessTypeUv,
@@ -25,7 +26,8 @@ struct process {
   Stream in, out, err;
   process_exit_cb cb;
   internal_process_cb internal_exit_cb, internal_close_cb;
-  bool closed, detach;
+  bool closed;
+  TriState detach;  // None=no_setsid, False=setsid, True=setsid+forget
   MultiQueue *events;
 };
 
@@ -50,7 +52,7 @@ static inline Process process_init(Loop *loop, ProcessType type, void *data)
     .closed = false,
     .internal_close_cb = NULL,
     .internal_exit_cb = NULL,
-    .detach = false
+    .detach = kFalse,
   };
 }
 

src/nvim/globals.h

@@ -72,12 +72,6 @@
 # define VIMRC_FILE     ".nvimrc"
 #endif
 
-typedef enum {
-  kNone  = -1,
-  kFalse = 0,
-  kTrue  = 1,
-} TriState;
-
 EXTERN struct nvim_stats_s {
   int64_t fsync;
   int64_t redraw;

src/nvim/os/process.c

@@ -68,6 +68,7 @@ static bool os_proc_tree_kill_rec(HANDLE process, int sig)
   }
 
 theend:
+  // TODO(justinmk): WaitForSingleObject to avoid race.
   return (bool)TerminateProcess(process, (unsigned int)sig);
 }
 /// Kills process `pid` and its descendants recursively.
@@ -90,20 +91,22 @@ bool os_proc_tree_kill(int pid, int sig)
 {
   assert(sig == SIGTERM || sig == SIGKILL);
   int pgid = getpgid(pid);
-  if (pgid > 0) {  // Ignore error. Never kill self (pid=0).
-    if (pgid == pid) {
-      ILOG("sending %s to process group: -%d",
-           sig == SIGTERM ? "SIGTERM" : "SIGKILL", pgid);
-      int rv = uv_kill(-pgid, sig);
-      return rv == 0;
-    } else {
-      // Should never happen, because process_spawn() did setsid() in the child.
-      ELOG("pgid %d != pid %d", pgid, pid);
-    }
-  } else {
+  if (pgid < 0) {
+    ELOG("getpgid(%d) error: %d: %s", pid, errno, strerror(errno));
+    return false;
+  }
+  if (pgid == 0) {  // Never kill self (pid=0).
     ELOG("getpgid(%d) returned %d", pid, pgid);
+    return false;
   }
-  return false;
+  if (pgid != pid) {
+    // Should never happen, because process_spawn() did setsid() in the child.
+    ELOG("pgid %d != pid %d", pgid, pid);
+    return false;
+  }
+  ILOG("sending %s to process group: -%d",
+       sig == SIGTERM ? "SIGTERM" : "SIGKILL", pgid);
+  return 0 == uv_kill(-pgid, sig);
 }
 #endif
 

src/nvim/os/shell.c

@@ -108,7 +108,8 @@ int os_call_shell(char_u *cmd, ShellOpts opts, char_u *extra_args)
   int current_state = State;
   bool forward_output = true;
 
-  // While the child is running, ignore terminating signals
+  // While the child is running, ignore terminating signals. process_wait()
+  // signals the process group (which includes nvim). #8217
   signal_reject_deadly();
 
   if (opts & (kShellOptHideMess | kShellOptExpand)) {
@@ -177,9 +178,13 @@ int os_system(char **argv,
               const char *input,
               size_t len,
               char **output,
-              size_t *nread) FUNC_ATTR_NONNULL_ARG(1)
+              size_t *nread)
+  FUNC_ATTR_NONNULL_ARG(1)
 {
-  return do_os_system(argv, input, len, output, nread, true, false);
+  signal_reject_deadly();
+  int exitcode = do_os_system(argv, input, len, output, nread, true, false);
+  signal_accept_deadly();
+  return exitcode;
 }
 
 static int do_os_system(char **argv,
@@ -216,6 +221,7 @@ static int do_os_system(char **argv,
   MultiQueue *events = multiqueue_new_child(main_loop.events);
   proc->events = events;
   proc->argv = argv;
+  proc->detach = kNone;  // No setsid(). #8217
   int status = process_spawn(proc, has_input, true, true);
   if (status) {
     loop_poll_events(&main_loop, 0);

src/nvim/types.h

@@ -15,4 +15,10 @@ typedef uint32_t u8char_T;
 
 typedef struct expand expand_T;
 
+typedef enum {
+  kNone  = -1,
+  kFalse = 0,
+  kTrue  = 1,
+} TriState;
+
 #endif  // NVIM_TYPES_H

test/functional/eval/system_spec.lua

@@ -6,6 +6,7 @@ local eq, call, clear, eval, feed_command, feed, nvim =
   helpers.feed, helpers.nvim
 local command = helpers.command
 local exc_exec = helpers.exc_exec
+local matches = helpers.matches
 local iswin = helpers.iswin
 
 local Screen = require('test.functional.ui.screen')
@@ -34,6 +35,19 @@ end
 describe('system()', function()
   before_each(clear)
 
+  it('runs in the same process-group as Nvim #8217', function()
+    if iswin() then
+      return  -- Not applicable.
+    end
+    local pid = tostring(eval('getpid()'))
+    -- system({string})
+    local out = eval([[filter(split(system('ps'), "\n"), 'v:val =~# "\\<'.getpid().'\\>"')]])
+    matches(pid..'.*nvim', out[1])
+    -- system({list})
+    out = eval([[filter(split(system(['ps']), "\n"), 'v:val =~# "\\<'.getpid().'\\>"')]])
+    matches(pid..'.*nvim', out[1])
+  end)
+
   describe('command passed as a List', function()
     local function printargs_path()
       return nvim_dir..'/printargs-test' .. (iswin() and '.exe' or '')
@@ -360,6 +374,19 @@ describe('systemlist()', function()
   -- Similar to `system()`, but returns List instead of String.
   before_each(clear)
 
+  it('runs in the same process-group as Nvim #8217', function()
+    if iswin() then
+      return  -- Not applicable.
+    end
+    local pid = tostring(eval('getpid()'))
+    -- systemlist({string})
+    local out = eval([[filter(systemlist('ps'), 'v:val =~# "\\<'.getpid().'\\>"')]])
+    matches(pid..'.*nvim', out[1])
+    -- systemlist({list})
+    out = eval([[filter(systemlist(['ps']), 'v:val =~# "\\<'.getpid().'\\>"')]])
+    matches(pid..'.*nvim', out[1])
+  end)
+
   it('sets v:shell_error', function()
     if iswin() then
       eval([[systemlist("cmd.exe /c exit")]])