Merge #8273 'job-control: avoid kill-timer race'

This commit is contained in:
Justin M. Keyes 2018-04-16 08:44:28 +02:00 committed by GitHub
commit ed6a113804
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 34 additions and 44 deletions

View File

@ -5033,10 +5033,9 @@ jobstart({cmd}[, {opts}]) *jobstart()*
width : (pty only) Width of the terminal screen width : (pty only) Width of the terminal screen
height : (pty only) Height of the terminal screen height : (pty only) Height of the terminal screen
TERM : (pty only) $TERM environment variable TERM : (pty only) $TERM environment variable
detach : (non-pty only) Detach the job process from the detach : (non-pty only) Detach the job process: it will
nvim process. The process will not get killed not be killed when Nvim exits. If the process
when nvim exits. If the process dies before exits before Nvim, "on_exit" will be invoked.
nvim exits, "on_exit" will still be invoked.
{opts} is passed as |self| dictionary to the callback; the {opts} is passed as |self| dictionary to the callback; the
caller may set other keys to pass application-specific data. caller may set other keys to pass application-specific data.

View File

@ -658,9 +658,9 @@ static void channel_process_exit_cb(Process *proc, int status, void *data)
terminal_close(chan->term, msg); terminal_close(chan->term, msg);
} }
// if status is -1 the process did not really exit, // If process did not exit, we only closed the handle of a detached process.
// we just closed the handle onto a detached process bool exited = (status >= 0);
if (status >= 0) { if (exited) {
process_channel_event(chan, &chan->on_exit, "exit", NULL, 0, status); process_channel_event(chan, &chan->on_exit, "exit", NULL, 0, status);
} }

View File

@ -21,7 +21,6 @@ void loop_init(Loop *loop, void *data)
loop->recursive = 0; loop->recursive = 0;
loop->uv.data = loop; loop->uv.data = loop;
loop->children = kl_init(WatcherPtr); loop->children = kl_init(WatcherPtr);
loop->children_stop_requests = 0;
loop->events = multiqueue_new_parent(loop_on_put, loop); loop->events = multiqueue_new_parent(loop_on_put, loop);
loop->fast_events = multiqueue_new_child(loop->events); loop->fast_events = multiqueue_new_child(loop->events);
loop->thread_events = multiqueue_new_parent(NULL, NULL); loop->thread_events = multiqueue_new_parent(NULL, NULL);

View File

@ -37,7 +37,6 @@ typedef struct loop {
// generic timer, used by loop_poll_events() // generic timer, used by loop_poll_events()
uv_timer_t poll_timer; uv_timer_t poll_timer;
size_t children_stop_requests;
uv_async_t async; uv_async_t async;
uv_mutex_t mutex; uv_mutex_t mutex;
int recursive; int recursive;

View File

@ -23,7 +23,7 @@
#endif #endif
// Time for a process to exit cleanly before we send KILL. // Time for a process to exit cleanly before we send KILL.
// For pty processes SIGTERM is sent first (in case SIGHUP was not enough). // For PTY processes SIGTERM is sent first (in case SIGHUP was not enough).
#define KILL_TIMEOUT_MS 2000 #define KILL_TIMEOUT_MS 2000
static bool process_is_tearing_down = false; static bool process_is_tearing_down = false;
@ -111,6 +111,7 @@ int process_spawn(Process *proc, bool in, bool out, bool err)
proc->internal_close_cb = decref; proc->internal_close_cb = decref;
proc->refcount++; proc->refcount++;
kl_push(WatcherPtr, proc->loop->children, proc); kl_push(WatcherPtr, proc->loop->children, proc);
DLOG("new: pid=%d argv=[%s]", proc->pid, *proc->argv);
return 0; return 0;
} }
@ -188,8 +189,7 @@ int process_wait(Process *proc, int ms, MultiQueue *events)
} }
if (proc->refcount == 1) { if (proc->refcount == 1) {
// Job exited, collect status and manually invoke close_cb to free the job // Job exited, free its resources.
// resources
decref(proc); decref(proc);
if (events) { if (events) {
// the decref call created an exit event, process it now // the decref call created an exit event, process it now
@ -205,11 +205,12 @@ int process_wait(Process *proc, int ms, MultiQueue *events)
/// Ask a process to terminate and eventually kill if it doesn't respond /// Ask a process to terminate and eventually kill if it doesn't respond
void process_stop(Process *proc) FUNC_ATTR_NONNULL_ALL void process_stop(Process *proc) FUNC_ATTR_NONNULL_ALL
{ {
if (proc->stopped_time) { bool exited = (proc->status >= 0);
if (exited || proc->stopped_time) {
return; return;
} }
proc->stopped_time = os_hrtime(); proc->stopped_time = os_hrtime();
switch (proc->type) { switch (proc->type) {
case kProcessTypeUv: case kProcessTypeUv:
// Close the process's stdin. If the process doesn't close its own // Close the process's stdin. If the process doesn't close its own
@ -227,35 +228,32 @@ void process_stop(Process *proc) FUNC_ATTR_NONNULL_ALL
abort(); abort();
} }
Loop *loop = proc->loop; // (Re)start timer to verify that stopped process(es) died.
if (!loop->children_stop_requests++) { uv_timer_start(&proc->loop->children_kill_timer, children_kill_cb,
// When there's at least one stop request pending, start a timer that KILL_TIMEOUT_MS, 0);
// will periodically check if a signal should be send to the job.
ILOG("starting job kill timer");
uv_timer_start(&loop->children_kill_timer, children_kill_cb,
KILL_TIMEOUT_MS, KILL_TIMEOUT_MS);
}
} }
/// Iterates the process list sending SIGTERM to stopped processes and SIGKILL /// Sends SIGKILL (or SIGTERM..SIGKILL for PTY jobs) to processes that did
/// to those that didn't die from SIGTERM after a while(exit_timeout is 0). /// not terminate after process_stop().
static void children_kill_cb(uv_timer_t *handle) static void children_kill_cb(uv_timer_t *handle)
{ {
Loop *loop = handle->loop->data; Loop *loop = handle->loop->data;
uint64_t now = os_hrtime();
kl_iter(WatcherPtr, loop->children, current) { kl_iter(WatcherPtr, loop->children, current) {
Process *proc = (*current)->data; Process *proc = (*current)->data;
if (!proc->stopped_time) { bool exited = (proc->status >= 0);
if (exited || !proc->stopped_time) {
continue; continue;
} }
uint64_t elapsed = (now - proc->stopped_time) / 1000000 + 1; uint64_t term_sent = UINT64_MAX == proc->stopped_time;
if (kProcessTypePty != proc->type || term_sent) {
if (elapsed >= KILL_TIMEOUT_MS) { os_proc_tree_kill(proc->pid, SIGKILL);
int sig = proc->type == kProcessTypePty && elapsed < KILL_TIMEOUT_MS * 2 } else {
? SIGTERM os_proc_tree_kill(proc->pid, SIGTERM);
: SIGKILL; proc->stopped_time = UINT64_MAX; // Flag: SIGTERM was sent.
os_proc_tree_kill(proc->pid, sig); // Restart timer.
uv_timer_start(&proc->loop->children_kill_timer, children_kill_cb,
KILL_TIMEOUT_MS, 0);
} }
} }
} }
@ -383,12 +381,8 @@ static void process_close_handles(void **argv)
static void on_process_exit(Process *proc) static void on_process_exit(Process *proc)
{ {
Loop *loop = proc->loop; Loop *loop = proc->loop;
if (proc->stopped_time && loop->children_stop_requests ILOG("exited: pid=%d status=%d stoptime=%" PRIu64, proc->pid, proc->status,
&& !--loop->children_stop_requests) { proc->stopped_time);
// Stop the timer if no more stop requests are pending
DLOG("Stopping process kill timer");
uv_timer_stop(&loop->children_kill_timer);
}
// Process has terminated, but there could still be data to be read from the // Process has terminated, but there could still be data to be read from the
// OS. We are still in the libuv loop, so we cannot call code that polls for // OS. We are still in the libuv loop, so we cannot call code that polls for

View File

@ -19,8 +19,7 @@ struct process {
Loop *loop; Loop *loop;
void *data; void *data;
int pid, status, refcount; int pid, status, refcount;
// set to the hrtime of when process_stop was called for the process. uint64_t stopped_time; // process_stop() timestamp
uint64_t stopped_time;
const char *cwd; const char *cwd;
char **argv; char **argv;
Stream in, out, err; Stream in, out, err;

View File

@ -32,7 +32,7 @@ describe('TermClose event', function()
end) end)
it('kills job trapping SIGTERM', function() it('kills job trapping SIGTERM', function()
if helpers.pending_win32(pending) then return end if iswin() then return end
nvim('set_option', 'shell', 'sh') nvim('set_option', 'shell', 'sh')
nvim('set_option', 'shellcmdflag', '-c') nvim('set_option', 'shellcmdflag', '-c')
command([[ let g:test_job = jobstart('trap "" TERM && echo 1 && sleep 60', { ]] command([[ let g:test_job = jobstart('trap "" TERM && echo 1 && sleep 60', { ]]
@ -51,8 +51,8 @@ describe('TermClose event', function()
ok(duration <= 4000) -- Epsilon for slow CI ok(duration <= 4000) -- Epsilon for slow CI
end) end)
it('kills pty job trapping SIGHUP and SIGTERM', function() it('kills PTY job trapping SIGHUP and SIGTERM', function()
if helpers.pending_win32(pending) then return end if iswin() then return end
nvim('set_option', 'shell', 'sh') nvim('set_option', 'shell', 'sh')
nvim('set_option', 'shellcmdflag', '-c') nvim('set_option', 'shellcmdflag', '-c')
command([[ let g:test_job = jobstart('trap "" HUP TERM && echo 1 && sleep 60', { ]] command([[ let g:test_job = jobstart('trap "" HUP TERM && echo 1 && sleep 60', { ]]