From 072639614655f3da5eacfe165c1faf5230206e5b Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 15 May 2018 02:56:50 +0200 Subject: [PATCH] job-control: do system() and :! in Nvim's process-group (This commit is for reference; the functional change will be reverted.) ref #8217 ref #8450 ref #8678 In terminal-Vim, system() and :! run in Vim's process-group. But 8d90171f8be6 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 8d90171f8be6 (#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 | 2 +- src/nvim/event/libuv_process.c | 9 ++++++--- src/nvim/event/process.c | 28 ++++++++++++++++++---------- src/nvim/event/process.h | 6 ++++-- src/nvim/globals.h | 6 ------ src/nvim/os/process.c | 27 +++++++++++++++------------ src/nvim/os/shell.c | 12 +++++++++--- src/nvim/types.h | 6 ++++++ test/functional/eval/system_spec.lua | 27 +++++++++++++++++++++++++++ 9 files changed, 86 insertions(+), 37 deletions(-) diff --git a/src/nvim/channel.c b/src/nvim/channel.c index 6ad64bbb85c31e..b8bae556ed42cf 100644 --- a/src/nvim/channel.c +++ b/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]); diff --git a/src/nvim/event/libuv_process.c b/src/nvim/event/libuv_process.c index ffe2db9b768ec2..f697bf5601f4e4 100644 --- a/src/nvim/event/libuv_process.c +++ b/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; diff --git a/src/nvim/event/process.c b/src/nvim/event/process.c index 7a8a39dbcf3bde..016c1548317be7 100644 --- a/src/nvim/event/process.c +++ b/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)); } diff --git a/src/nvim/event/process.h b/src/nvim/event/process.h index ba2c2a6a1185a2..aa61123363aef0 100644 --- a/src/nvim/event/process.h +++ b/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, }; } diff --git a/src/nvim/globals.h b/src/nvim/globals.h index d9103f516cd435..74be449556e400 100644 --- a/src/nvim/globals.h +++ b/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; diff --git a/src/nvim/os/process.c b/src/nvim/os/process.c index a67e7487eb645f..7a7679a8c884bd 100644 --- a/src/nvim/os/process.c +++ b/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 diff --git a/src/nvim/os/shell.c b/src/nvim/os/shell.c index 04f59d75229b39..171fc82836c6d4 100644 --- a/src/nvim/os/shell.c +++ b/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); diff --git a/src/nvim/types.h b/src/nvim/types.h index 317bead3bb677d..e05debed297673 100644 --- a/src/nvim/types.h +++ b/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 diff --git a/test/functional/eval/system_spec.lua b/test/functional/eval/system_spec.lua index 5e12b6a6a4b2b2..d807b6eb80253d 100644 --- a/test/functional/eval/system_spec.lua +++ b/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")]])