From a2e3e4d8597279617fda5ff819393b87e211bc4e Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Fri, 16 Feb 2018 19:00:02 +0100 Subject: [PATCH] jobs: child proc must have a separate process-group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UV_PROCESS_DETACHED compels libuv:uv__process_child_init() to call setsid() in the child just after fork(). That ensures the process and its descendants are grouped in a separate session (and process group). The following jobstart() call correctly groups `sh` and `sleep` in a new session (and process-group), where `sh` is the "session leader" (and process-group leader): :call jobstart(['sh','-c','sleep 60']) SESN PGRP PID PPID USER Command 30383 30383 30383 3620 vagrant │ ├─ -bash 30383 31432 31432 30383 vagrant │ │ └─ nvim -u NORC 30383 31432 31433 30383 vagrant │ │ ├─ nvim -u NORC 8105 8105 8105 31432 vagrant │ │ └─ sh -c sleep 60 8105 8105 8106 8105 vagrant │ │ └─ sleep 60 closes #6530 ref: https://stackoverflow.com/q/1046933 ref: https://unix.stackexchange.com/a/404065 Helped-by: Marco Hinz Discussion ------------------------------------------------------------------------ On my linux box before this patch, the termclose_spec.lua:'kills job trapping SIGTERM' test indirectly causes cmake/busted to wait for 60s. That's because the test spawns a `sleep 60` descendant process which hangs around even after nvim exits: nvim killed the parent PID, but not PGID (process-group), so the grandchild "reparented" to init (PID 1). Session contains processes (and process-groups) which are logically part of the same "login session". Process-group is a set of logically/informally-related processes within a session; for example, shells assign a process group to each "job". Session IDs and PGIDs both have type pid_t (like PIDs). These OS-level mechanisms are, as usual, legacy accidents whose purpose is upheld by convention and folklore. We can use session-level grouping (setsid), or we could use process-group-level grouping (setpgid). Vim uses setsid() if available, otherwise setpgid(0,0). Windows ------------------------------------------------------------------------ UV_PROCESS_DETACHED on win32 sets CREATE_NEW_PROCESS_GROUP flag. But uv_kill() does not kill the process-group: https://github.com/nodejs/node/issues/3617 Ideas: - Set UV_PROCESS_WINDOWS_HIDE (CREATE_NEW_PROCESS_GROUP), then call GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, pid) - Maybe won't work because MSDN says "Only processes that share the same console as the calling process receive the signal." https://docs.microsoft.com/en-us/windows/console/generateconsolectrlevent But CREATE_NEW_PROCESS_GROUP creates a new console ... ref https://stackoverflow.com/q/1453520 - Group processes within a "job". libuv does that *globally* for non-detached processes: uv__init_global_job_handle. - Iterate through CreateToolhelp32Snapshot(). - https://stackoverflow.com/q/1173342 - Vim does this, see terminate_all() TODO ------------------------------------------------------------------------ - test-case from #6530: call jobstop(jobstart('/bin/bash -c ''trap true SIGTERM; sleep 20000000000000''')) - win: iterate through children - implement nvim_get_proc_children(): https://github.com/libuv/libuv/pull/836 - remove useless "watched children" (`loop->children`) from `children_kill_cb` ? - get parent process id: uv_os_getppid() (libuv 1.16+) --- src/nvim/event/libuv_process.c | 9 ++++++--- src/nvim/event/process.c | 31 +++++++++++++++++++++++-------- src/nvim/os/pty_process_unix.c | 6 +++++- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/nvim/event/libuv_process.c b/src/nvim/event/libuv_process.c index c101cb1bb9e957..5080f6b1d19a66 100644 --- a/src/nvim/event/libuv_process.c +++ b/src/nvim/event/libuv_process.c @@ -26,15 +26,18 @@ int libuv_process_spawn(LibuvProcess *uvproc) uvproc->uvopts.file = proc->argv[0]; uvproc->uvopts.args = proc->argv; uvproc->uvopts.flags = UV_PROCESS_WINDOWS_HIDE; - if (proc->detach) { - uvproc->uvopts.flags |= UV_PROCESS_DETACHED; - } #ifdef WIN32 // libuv collapses the argv to a CommandLineToArgvW()-style string. cmd.exe // expects a different syntax (must be prepared by the caller before now). if (os_shell_is_cmdexe(proc->argv[0])) { uvproc->uvopts.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS; } + if (proc->detach) { + uvproc->uvopts.flags |= UV_PROCESS_DETACHED; + } +#else + // Always setsid() on unix-likes. + 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 a06f5f4ff3d2ce..d6b606fbbbfbe5 100644 --- a/src/nvim/event/process.c +++ b/src/nvim/event/process.c @@ -209,14 +209,22 @@ void process_stop(Process *proc) FUNC_ATTR_NONNULL_ALL } proc->stopped_time = os_hrtime(); + int pgid = getpgid(proc->pid); switch (proc->type) { 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); - ILOG("Sending SIGTERM to pid %d", proc->pid); - uv_kill(proc->pid, SIGTERM); + if (pgid > 0) { // Ignore error. Never kill self (0). + if (pgid == proc->pid) { // Should always be true. + ILOG("sending SIGTERM to pid: %d", proc->pid); + uv_kill(-pgid, SIGTERM); + } else { + // Should never happen, process_spawn() does setsid() in the child. + ELOG("pgid %d != pid %d", pgid, proc->pid); + } + } break; case kProcessTypePty: // close all streams for pty processes to send SIGHUP to the process @@ -231,7 +239,7 @@ void process_stop(Process *proc) FUNC_ATTR_NONNULL_ALL if (!loop->children_stop_requests++) { // When there's at least one stop request pending, start a timer that // will periodically check if a signal should be send to the job. - ILOG("Starting job kill timer"); + ILOG("starting job kill timer"); uv_timer_start(&loop->children_kill_timer, children_kill_cb, KILL_TIMEOUT_MS, KILL_TIMEOUT_MS); } @@ -253,11 +261,18 @@ static void children_kill_cb(uv_timer_t *handle) if (elapsed >= KILL_TIMEOUT_MS) { int sig = proc->type == kProcessTypePty && elapsed < KILL_TIMEOUT_MS * 2 - ? SIGTERM - : SIGKILL; - ILOG("Sending %s to pid %d", sig == SIGTERM ? "SIGTERM" : "SIGKILL", - proc->pid); - uv_kill(proc->pid, sig); + ? SIGTERM + : SIGKILL; + int pgid = getpgid(proc->pid); + if (pgid > 0) { // Ignore error. Never kill self (0). + if (pgid == proc->pid) { // Should always be true. + ILOG("sending %s to PID: %d", sig == SIGTERM ? "SIGTERM" : "SIGKILL", pgid); + uv_kill(-pgid, sig); + } else { + // Should never happen, process_spawn() does setsid() in the child. + ELOG("pgid %d != pid %d", pgid, proc->pid); + } + } } } } diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c index 855ca2ae476e7b..dbf4d7d9b99746 100644 --- a/src/nvim/os/pty_process_unix.c +++ b/src/nvim/os/pty_process_unix.c @@ -145,8 +145,12 @@ void pty_process_teardown(Loop *loop) uv_signal_stop(&loop->children_watcher); } -static void init_child(PtyProcess *ptyproc) FUNC_ATTR_NONNULL_ALL +static void init_child(PtyProcess *ptyproc) + FUNC_ATTR_NONNULL_ALL // FUNC_ATTR_NORETURN ? { + // New session and progress-group. #6530 + setsid(); + unsetenv("COLUMNS"); unsetenv("LINES"); unsetenv("TERMCAP");