diff --git a/.Rbuildignore b/.Rbuildignore index 3715cd14..4d1dcdcb 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -11,6 +11,8 @@ ^src/processx\.dll$ ^src/client\.so$ ^src/client\.dll$ +^src/test/sigtermignore\.so$ +^src/test/sigtermignore\.dll$ ^src/.*\.o$ ^src/tools/px$ ^src/tools/px.exe$ diff --git a/DESCRIPTION b/DESCRIPTION index 48955646..8fabaa30 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -33,6 +33,7 @@ Suggests: curl, debugme, parallel, + pkgload, rlang (>= 1.0.2), testthat (>= 3.0.0), webfakes, diff --git a/NEWS.md b/NEWS.md index e27f25a7..39f2e0a2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,15 @@ # processx (development version) +* The `grace` argument of the `kill()` method is now active on Unix + platforms. processx first tries to kill with `SIGTERM` with a + timeout of `grace` seconds. After the timeout, `SIGKILL` is sent as + before. + +* New `cleanup_grace` argument to `process$new()` and `run()`. When the + process is cleaned up (on GC or on exit), processx first tries + `SIGTERM` with a timeout of `cleanup_grace` seconds before falling + back to `SIGKILL`. On Unix only. + # processx 3.9.0 * New experimental `pipeline` R6 class for running two or more processes diff --git a/R/assertions.R b/R/assertions.R index aacff833..8d5d6568 100644 --- a/R/assertions.R +++ b/R/assertions.R @@ -26,6 +26,14 @@ on_failure(is_flag) <- function(call, env) { paste0(deparse(call$x), " is not a flag (length 1 logical)") } +is_nonneg_numeric_scalar <- function(x) { + is.numeric(x) && length(x) == 1 && !is.na(x) && x >= 0 +} + +on_failure(is_nonneg_numeric_scalar) <- function(call, env) { + paste0(deparse(call$x), " is not a non-negative length 1 number") +} + is_integerish_scalar <- function(x) { is.numeric(x) && length(x) == 1 && !is.na(x) && round(x) == x } diff --git a/R/initialize.R b/R/initialize.R index 1d2f47b1..30ddaa0a 100644 --- a/R/initialize.R +++ b/R/initialize.R @@ -36,6 +36,7 @@ process_initialize <- function( env, cleanup, cleanup_tree, + cleanup_grace, wd, echo_cmd, supervise, @@ -62,6 +63,7 @@ process_initialize <- function( is.null(env) || is_env_vector(env), is_flag(cleanup), is_flag(cleanup_tree), + is_nonneg_numeric_scalar(cleanup_grace), is_string_or_null(wd), is_flag(echo_cmd), is_flag(windows_verbatim_args), @@ -119,6 +121,7 @@ process_initialize <- function( private$args <- args private$cleanup <- cleanup private$cleanup_tree <- cleanup_tree + private$cleanup_grace <- cleanup_grace private$wd <- wd private$pstdin <- stdin private$pstdout <- stdout @@ -190,6 +193,7 @@ process_initialize <- function( windows_detached_process, private, cleanup, + cleanup_grace, wd, encoding, paste0("PROCESSX_", private$tree_id, "=YES"), diff --git a/R/process.R b/R/process.R index a492a0ad..daf77a7d 100644 --- a/R/process.R +++ b/R/process.R @@ -229,6 +229,10 @@ process <- R6::R6Class( #' object is garbage collected. #' @param cleanup_tree Whether to kill the process and its child #' process tree when the `process` object is garbage collected. + #' @param cleanup_grace Grace period between `SIGTERM` and `SIGKILL`. + #' Only has an effect on Unix platforms. Set to 0 to terminate abruptly + #' with `SIGKILL` only. Currently defaults to 0 until we implement + #' a better approach on session quit. #' @param wd Working directory of the process. It must exist. #' If `NULL`, then the current working directory is used. #' @param echo_cmd Whether to print the command to the screen before @@ -275,6 +279,7 @@ process <- R6::R6Class( env = NULL, cleanup = TRUE, cleanup_tree = FALSE, + cleanup_grace = 0.0, wd = NULL, echo_cmd = FALSE, supervise = FALSE, @@ -300,6 +305,7 @@ process <- R6::R6Class( env, cleanup, cleanup_tree, + cleanup_grace, wd, echo_cmd, supervise, @@ -758,6 +764,7 @@ process <- R6::R6Class( args = NULL, # Save 'args' argument here cleanup = NULL, # cleanup argument cleanup_tree = NULL, # cleanup_tree argument + cleanup_grace = NULL, # cleanup_grace argument stdin = NULL, # stdin argument or stream stdout = NULL, # stdout argument or stream stderr = NULL, # stderr argument or stream @@ -862,7 +869,7 @@ process_interrupt <- function(self, private) { process_kill <- function(self, private, grace, close_connections) { "!DEBUG process_kill '`private$get_short_name()`', pid `self$get_pid()`" - ret <- chain_call( + ret <- chain_clean_call( c_processx_kill, private$status, as.numeric(grace), diff --git a/R/run.R b/R/run.R index 00dd7bed..5e401da0 100644 --- a/R/run.R +++ b/R/run.R @@ -126,6 +126,7 @@ #' strings. Line callbacks are not supported in binary mode. #' @param cleanup_tree Whether to clean up the child process tree after #' the process has finished. +#' @param cleanup_grace Passed to `kill()` or `kill_tree()` on cleanup. #' @param stdin What to do with the standard input. By default it is #' ignored (`NULL`). It can be a file name, to redirect the contents of #' a file to the standard input. When `pty = TRUE`, `stdin` can only be @@ -195,6 +196,7 @@ run <- function( windows_hide_window = FALSE, encoding = "", cleanup_tree = FALSE, + cleanup_grace = 0.1, pty = FALSE, pty_options = list(), ... @@ -215,6 +217,7 @@ run <- function( assert_that(is.null(stdout_callback) || is.function(stdout_callback)) assert_that(is.null(stderr_callback) || is.function(stderr_callback)) assert_that(is_flag(cleanup_tree)) + assert_that(is_nonneg_numeric_scalar(cleanup_grace)) assert_that(is_flag(stderr_to_stdout)) if (encoding == "binary") { if (!is.null(stdout_line_callback)) { @@ -299,9 +302,9 @@ run <- function( ## We make sure that the process is eliminated if (cleanup_tree) { - on.exit(pr$kill_tree(), add = TRUE) + defer(pr$kill_tree(grace = cleanup_grace)) } else { - on.exit(pr$kill(), add = TRUE) + defer(pr$kill(grace = cleanup_grace)) } ## If echo, then we need to create our own callbacks. @@ -365,7 +368,7 @@ run <- function( } resenv$errbuf$read() } - tryCatch(pr$kill(), error = function(e) NULL) + tryCatch(pr$kill(grace = cleanup_grace), error = function(e) NULL) signalCondition(new_process_interrupt_cond( list( interrupt = TRUE, diff --git a/R/utils.R b/R/utils.R index 4b4b215e..a7b8eb8d 100644 --- a/R/utils.R +++ b/R/utils.R @@ -352,3 +352,14 @@ ends_with <- function(x, post) { substr(x, nchar(x) - l + 1, nchar(x)) == post } +defer <- function(expr, frame = parent.frame(), after = FALSE) { + thunk <- as.call(list(function() expr)) + do.call(on.exit, list(thunk, add = TRUE, after = after), envir = frame) +} + +rimraf <- function(...) { + x <- file.path(...) + if ("~" %in% x) stop("Cowardly refusing to delete `~`") + unlink(x, recursive = TRUE, force = TRUE) +} + diff --git a/man/process.Rd b/man/process.Rd index 1a6ce99a..c08082f3 100644 --- a/man/process.Rd +++ b/man/process.Rd @@ -206,6 +206,7 @@ Start a new process in the background, and then return immediately. env = NULL, cleanup = TRUE, cleanup_tree = FALSE, + cleanup_grace = 0, wd = NULL, echo_cmd = FALSE, supervise = FALSE, @@ -327,6 +328,11 @@ object is garbage collected.} \item{\code{cleanup_tree}}{Whether to kill the process and its child process tree when the \code{process} object is garbage collected.} +\item{\code{cleanup_grace}}{Grace period between \code{SIGTERM} and \code{SIGKILL}. +Only has an effect on Unix platforms. Set to 0 to terminate abruptly +with \code{SIGKILL} only. Currently defaults to 0 until we implement +a better approach on session quit.} + \item{\code{wd}}{Working directory of the process. It must exist. If \code{NULL}, then the current working directory is used.} diff --git a/man/process_initialize.Rd b/man/process_initialize.Rd index 50256224..5fb0b468 100644 --- a/man/process_initialize.Rd +++ b/man/process_initialize.Rd @@ -19,6 +19,7 @@ process_initialize( env, cleanup, cleanup_tree, + cleanup_grace, wd, echo_cmd, supervise, diff --git a/man/run.Rd b/man/run.Rd index c61383b3..9cd39955 100644 --- a/man/run.Rd +++ b/man/run.Rd @@ -26,6 +26,7 @@ run( windows_hide_window = FALSE, encoding = "", cleanup_tree = FALSE, + cleanup_grace = 0.1, pty = FALSE, pty_options = list(), ... @@ -140,6 +141,8 @@ strings. Line callbacks are not supported in binary mode.} \item{cleanup_tree}{Whether to clean up the child process tree after the process has finished.} +\item{cleanup_grace}{Passed to \code{kill()} or \code{kill_tree()} on cleanup.} + \item{pty}{Whether to use a pseudo-terminal (PTY) for the process. Supported on Unix and on Windows 10 version 1809 or later (via ConPTY). When \code{TRUE}, stdout and stderr are merged into a single diff --git a/src/init.c b/src/init.c index b1ae202d..97a50f4a 100644 --- a/src/init.c +++ b/src/init.c @@ -97,7 +97,7 @@ SEXP is_valgrind_(void) static const R_CallMethodDef callMethods[] = { CLEANCALL_METHOD_RECORD, - { "processx_exec", (DL_FUNC) &processx_exec, 15 }, + { "processx_exec", (DL_FUNC) &processx_exec, 16 }, { "processx_wait", (DL_FUNC) &processx_wait, 3 }, { "processx_is_alive", (DL_FUNC) &processx_is_alive, 2 }, { "processx_pty_close", (DL_FUNC) &processx_pty_close, 2 }, diff --git a/src/processx.h b/src/processx.h index 4ecd088e..79acedbb 100644 --- a/src/processx.h +++ b/src/processx.h @@ -45,8 +45,8 @@ extern "C" { SEXP processx_exec(SEXP command, SEXP args, SEXP pty, SEXP pty_options, SEXP connections, SEXP env, SEXP windows_verbatim_args, SEXP windows_hide_window, SEXP windows_detached_process, - SEXP private_, SEXP cleanup, SEXP wd, SEXP encoding, - SEXP tree_id, SEXP linux_pdeathsig); + SEXP private_, SEXP cleanup, SEXP cleanup_grace, + SEXP wd, SEXP encoding, SEXP tree_id, SEXP linux_pdeathsig); SEXP processx_wait(SEXP status, SEXP timeout, SEXP name); SEXP processx_is_alive(SEXP status, SEXP name); SEXP processx_pty_close(SEXP status, SEXP name); diff --git a/src/tools/px.c b/src/tools/px.c index 46f71365..430b292c 100644 --- a/src/tools/px.c +++ b/src/tools/px.c @@ -50,6 +50,10 @@ void usage(void) { "write raw bytes (hex pairs) to stdout\n"); fprintf(stderr, " rawerr -- " "write raw bytes (hex pairs) to stderr\n"); + fprintf(stderr, " sigterm ignore -- " + "ignore SIGTERM\n"); + fprintf(stderr, " sigterm sleep -- " + "sleep a number of seconds on SIGTERM, then quit\n"); } void cat2(int f, const char *s) { @@ -112,6 +116,35 @@ int echo_from_fd(int fd1, int fd2, int nbytes) { return 0; } +#ifdef WIN32 +void sigterm_ignore(void) { } +void sigterm_sleep(double fnum) { } +#else +#include + +double sig_sleep_secs; + +void sigterm_ignore(void) { + signal(SIGTERM, SIG_IGN); +} + +void sig_handler_sleep(int sig, siginfo_t *info, void *ucontext) { + double fnum = sig_sleep_secs; + int num = (int) fnum; + sleep(num); + fnum = fnum - num; + if (fnum > 0) usleep((useconds_t)(fnum * 1000.0 * 1000.0)); +} + +void sigterm_sleep(double fnum) { + sig_sleep_secs = fnum; + struct sigaction sig = {{ 0 }}; + sig.sa_flags = SA_SIGINFO; + sig.sa_sigaction = &sig_handler_sleep; + sigaction(SIGTERM, &sig, NULL); +} +#endif + int main(int argc, const char **argv) { int num, idx, ret, fd, fd2, nbytes; @@ -258,6 +291,27 @@ int main(int argc, const char **argv) { } } + } else if (!strcmp("sigterm", cmd)) { + idx++; + if (!strcmp("ignore", argv[idx])) { + sigterm_ignore(); + } else if (!strcmp("sleep", argv[idx])) { + if (idx + 1 >= argc) { + fprintf(stderr, "Missing argument for 'sigterm sleep'\n"); + return 17; + } + idx++; + ret = sscanf(argv[idx], "%lf", &fnum); + if (ret != 1) { + fprintf(stderr, "Invalid seconds for px sigterm sleep: '%s'\n", argv[idx]); + return 18; + } + sigterm_sleep(fnum); + } else { + fprintf(stderr, "Invalid 'sigterm' subcommand\n"); + return 19; + } + } else { fprintf(stderr, "Unknown px command: '%s'\n", cmd); return 2; diff --git a/src/unix/processx-unix.h b/src/unix/processx-unix.h index 3bb3adc7..504b4c1a 100644 --- a/src/unix/processx-unix.h +++ b/src/unix/processx-unix.h @@ -23,6 +23,7 @@ typedef struct processx_handle_s { int fd2; /* readable */ int waitpipe[2]; /* use it for wait() with timeout */ int cleanup; + double cleanup_grace; double create_time; double end_time; /* 0.0 until the process exits */ processx_connection_t *pipes[3]; @@ -38,7 +39,12 @@ void processx__sigchld_callback(int sig, siginfo_t *info, void *ctx); void processx__setup_sigchld(void); void processx__remove_sigchld(void); void processx__block_sigchld(void); +void processx__block_sigchld_save(sigset_t *old); void processx__unblock_sigchld(void); +void processx__procmask_set(sigset_t *set); + +int c_processx_wait(processx_handle_t *handle, int timeout, const char *name); +int c_processx_kill(SEXP status, double grace, SEXP name); void processx__finalizer(SEXP status); diff --git a/src/unix/processx.c b/src/unix/processx.c index f4c8c7b5..5258979d 100644 --- a/src/unix/processx.c +++ b/src/unix/processx.c @@ -22,7 +22,7 @@ static void processx__child_init(processx_handle_t *handle, SEXP connections, processx_options_t *options, const char *tree_id); -static SEXP processx__make_handle(SEXP private, int cleanup); +static SEXP processx__make_handle(SEXP private, int cleanup, double cleanup_grace); static void processx__handle_destroy(processx_handle_t *handle); void processx__create_connections(processx_handle_t *handle, SEXP private, const char *encoding); @@ -349,38 +349,38 @@ static void processx__child_init(processx_handle_t *handle, SEXP connections, /* LCOV_EXCL_STOP */ +struct cleanup_kill_data { + SEXP status; + double grace; + SEXP name; +}; + +SEXP c_processx_kill_data(void *payload) { + struct cleanup_kill_data *data = (struct cleanup_kill_data *) payload; + c_processx_kill(data->status, data->grace, data->name); + return R_NilValue; +} + void processx__finalizer(SEXP status) { processx_handle_t *handle = (processx_handle_t*) R_ExternalPtrAddr(status); - pid_t pid; - int wp, wstat; - - processx__block_sigchld(); /* Free child list nodes that are not needed any more. */ + processx__block_sigchld(); processx__freelist_free(); + processx__unblock_sigchld(); /* Already freed? */ - if (!handle) goto cleanup; - - pid = handle->pid; + if (!handle) + return; + // FIXME: Do we need cleancall here? if (handle->cleanup) { - /* Do a non-blocking waitpid() to see if it is running */ - do { - wp = waitpid(pid, &wstat, WNOHANG); - } while (wp == -1 && errno == EINTR); - - /* Maybe just waited on it? Then collect status */ - if (wp == pid) processx__collect_exit_status(status, wp, wstat); - - /* If it is running, we need to kill it, and wait for the exit status */ - if (wp == 0) { - kill(-pid, SIGKILL); - do { - wp = waitpid(pid, &wstat, 0); - } while (wp == -1 && errno == EINTR); - processx__collect_exit_status(status, wp, wstat); - } + struct cleanup_kill_data data = { + .status = status, + .grace = handle->cleanup_grace, + .name = R_NilValue + }; + r_with_cleanup_context(c_processx_kill_data, &data); } /* Note: if no cleanup is requested, then we still have a sigchld @@ -390,12 +390,9 @@ void processx__finalizer(SEXP status) { /* Deallocate memory */ R_ClearExternalPtr(status); processx__handle_destroy(handle); - - cleanup: - processx__unblock_sigchld(); } -static SEXP processx__make_handle(SEXP private, int cleanup) { +static SEXP processx__make_handle(SEXP private, int cleanup, double cleanup_grace) { processx_handle_t * handle; SEXP result; @@ -407,6 +404,7 @@ static SEXP processx__make_handle(SEXP private, int cleanup) { result = PROTECT(R_MakeExternalPtr(handle, private, R_NilValue)); R_RegisterCFinalizerEx(result, processx__finalizer, 1); handle->cleanup = cleanup; + handle->cleanup_grace = cleanup_grace; UNPROTECT(1); return result; @@ -454,13 +452,14 @@ void processx__make_socketpair(int pipe[2], const char *exe) { SEXP processx_exec(SEXP command, SEXP args, SEXP pty, SEXP pty_options, SEXP connections, SEXP env, SEXP windows_verbatim_args, SEXP windows_hide_window, SEXP windows_detached_process, - SEXP private, SEXP cleanup, SEXP wd, SEXP encoding, - SEXP tree_id, SEXP linux_pdeathsig) { + SEXP private, SEXP cleanup, SEXP cleanup_grace, SEXP wd, + SEXP encoding, SEXP tree_id, SEXP linux_pdeathsig) { char *ccommand = processx__tmp_string(command, 0); char **cargs = processx__tmp_character(args); char **cenv = isNull(env) ? 0 : processx__tmp_character(env); int ccleanup = INTEGER(cleanup)[0]; + double ccleanup_grace = REAL(cleanup_grace)[0]; const int cpty = LOGICAL(pty)[0]; const char *cencoding = CHAR(STRING_ELT(encoding, 0)); @@ -496,7 +495,7 @@ SEXP processx_exec(SEXP command, SEXP args, SEXP pty, SEXP pty_options, processx__setup_sigchld(); - result = PROTECT(processx__make_handle(private, ccleanup)); + result = PROTECT(processx__make_handle(private, ccleanup, ccleanup_grace)); handle = R_ExternalPtrAddr(result); if (cpty) { @@ -736,30 +735,38 @@ SEXP processx_pty_close(SEXP status, SEXP name) { SEXP processx_wait(SEXP status, SEXP timeout, SEXP name) { processx_handle_t *handle = R_ExternalPtrAddr(status); + int ctimeout = INTEGER(timeout)[0]; const char *cname = isNull(name) ? "???" : CHAR(STRING_ELT(name, 0)); - int ctimeout = INTEGER(timeout)[0], timeleft = ctimeout; + + int ret = c_processx_wait(handle, ctimeout, cname); + return ScalarLogical(ret); +} + +int c_processx_wait(processx_handle_t *handle, int timeout, const char *name) { struct pollfd fd; int ret = 0; pid_t pid; + int timeleft = timeout; int *fds = malloc(sizeof(int) * 2); if (!fds) R_THROW_SYSTEM_ERROR("Allocating memory when waiting"); fds[0] = fds[1] = -1; r_call_on_exit(processx__wait_cleanup, fds); - processx__block_sigchld(); + sigset_t old; + processx__block_sigchld_save(&old); if (!handle) { - processx__unblock_sigchld(); - return ScalarLogical(1); + processx__procmask_set(&old); + return 1; } pid = handle->pid; /* If we already have the status, then return now. */ if (handle->collected) { - processx__unblock_sigchld(); - return ScalarLogical(1); + processx__procmask_set(&old); + return 1; } /* Make sure this is active, in case another package replaced it... */ @@ -768,8 +775,8 @@ SEXP processx_wait(SEXP status, SEXP timeout, SEXP name) { /* Setup the self-pipe that we can poll */ if (pipe(handle->waitpipe)) { - processx__unblock_sigchld(); - R_THROW_SYSTEM_ERROR("processx error when waiting for '%s'", cname); + processx__procmask_set(&old); + R_THROW_SYSTEM_ERROR("processx error when waiting for '%s'", name); } fds[0] = handle->waitpipe[0]; fds[1] = handle->waitpipe[1]; @@ -785,7 +792,7 @@ SEXP processx_wait(SEXP status, SEXP timeout, SEXP name) { - while (ctimeout < 0 || timeleft > PROCESSX_INTERRUPT_INTERVAL) { + while (timeout < 0 || timeleft > PROCESSX_INTERRUPT_INTERVAL) { do { ret = poll(&fd, 1, PROCESSX_INTERRUPT_INTERVAL); } while (ret == -1 && errno == EINTR); @@ -805,7 +812,7 @@ SEXP processx_wait(SEXP status, SEXP timeout, SEXP name) { goto cleanup; } - if (ctimeout >= 0) timeleft -= PROCESSX_INTERRUPT_INTERVAL; + if (timeout >= 0) timeleft -= PROCESSX_INTERRUPT_INTERVAL; } /* Maybe we are not done, and there is a little left from the timeout */ @@ -817,7 +824,7 @@ SEXP processx_wait(SEXP status, SEXP timeout, SEXP name) { if (ret == -1) { R_THROW_SYSTEM_ERROR("processx wait with timeout error while " - "waiting for '%s'", cname); + "waiting for '%s'", name); } cleanup: @@ -825,7 +832,9 @@ SEXP processx_wait(SEXP status, SEXP timeout, SEXP name) { handle->waitpipe[0] = -1; handle->waitpipe[1] = -1; - return ScalarLogical(ret != 0); + processx__procmask_set(&old); + + return ret != 0; } /* This is similar to `processx_wait`, but a bit simpler, because we @@ -1025,6 +1034,10 @@ SEXP processx_interrupt(SEXP status, SEXP name) { */ SEXP processx_kill(SEXP status, SEXP grace, SEXP name) { + return ScalarLogical(c_processx_kill(status, REAL(grace)[0], name)); +} + +int c_processx_kill(SEXP status, double grace, SEXP name) { processx_handle_t *handle = R_ExternalPtrAddr(status); const char *cname = isNull(name) ? "???" : CHAR(STRING_ELT(name, 0)); pid_t pid; @@ -1059,14 +1072,29 @@ SEXP processx_kill(SEXP status, SEXP grace, SEXP name) { /* If the process is not running, return (FALSE) */ if (wp != 0) { goto cleanup; } - /* It is still running, so a SIGKILL */ - int ret = kill(-pid, SIGKILL); - if (ret == -1 && (errno == ESRCH || errno == EPERM)) { goto cleanup; } - if (ret == -1) { - processx__unblock_sigchld(); - R_THROW_SYSTEM_ERROR("process_kill for '%s'", cname); + /* It is still running, send a SIGTERM if gracious */ + int grace_ms = (int)(grace * 1000); + +#define KILL_WITH(SIG) do { \ + int ret = kill(-pid, SIG); \ + if (ret == -1 && (errno == ESRCH || errno == EPERM)) { goto cleanup; } \ + if (ret == -1) { \ + processx__unblock_sigchld(); \ + R_THROW_SYSTEM_ERROR("process_kill for '%s'", cname); \ + } \ +} while(0) + + if (grace_ms) { + KILL_WITH(SIGTERM); + if (c_processx_wait(handle, grace_ms, cname)) { + result = handle->exitcode == -SIGTERM; + goto cleanup; + } } + /* It is still running, send a SIGKILL */ + KILL_WITH(SIGKILL); + /* Do a waitpid to collect the status and reap the zombie */ do { wp = waitpid(pid, &wstat, 0); @@ -1082,7 +1110,7 @@ SEXP processx_kill(SEXP status, SEXP grace, SEXP name) { cleanup: processx__unblock_sigchld(); - return ScalarLogical(result); + return result; } SEXP processx_get_pid(SEXP status) { diff --git a/src/unix/sigchld.c b/src/unix/sigchld.c index cad171bb..5b53e66b 100644 --- a/src/unix/sigchld.c +++ b/src/unix/sigchld.c @@ -128,15 +128,26 @@ void processx__remove_sigchld(void) { memset(&old_sig_handler, 0, sizeof(old_sig_handler)); } -void processx__block_sigchld(void) { +void processx__block_sigchld_save(sigset_t *old) { sigset_t blockMask; sigemptyset(&blockMask); sigaddset(&blockMask, SIGCHLD); - if (sigprocmask(SIG_BLOCK, &blockMask, NULL) == -1) { + + if (sigprocmask(SIG_BLOCK, &blockMask, old) == -1) { R_THROW_ERROR("processx error setting up signal handlers"); } } +void processx__procmask_set(sigset_t *set) { + if (sigprocmask(SIG_SETMASK, set, NULL) == -1) { + R_THROW_ERROR("processx error setting up signal handlers"); + } +} + +void processx__block_sigchld(void) { + processx__block_sigchld_save(NULL); +} + void processx__unblock_sigchld(void) { sigset_t unblockMask; sigemptyset(&unblockMask); diff --git a/src/win/processx-win.h b/src/win/processx-win.h index b533c9be..d60179ab 100644 --- a/src/win/processx-win.h +++ b/src/win/processx-win.h @@ -13,6 +13,7 @@ typedef struct processx_handle_s { HANDLE waitObject; processx_connection_t *pipes[3]; int cleanup; + double cleanup_grace; double create_time; double end_time; /* 0.0 until the process exits */ void *ptycon; /* ConPTY handle (HPCON), NULL if not using PTY */ diff --git a/src/win/processx.c b/src/win/processx.c index 62433d9e..8a865492 100644 --- a/src/win/processx.c +++ b/src/win/processx.c @@ -922,7 +922,7 @@ void processx__finalizer(SEXP status) { processx__handle_destroy(handle); } -SEXP processx__make_handle(SEXP private, int cleanup) { +SEXP processx__make_handle(SEXP private, int cleanup, double cleanup_grace) { processx_handle_t * handle; SEXP result; @@ -933,6 +933,7 @@ SEXP processx__make_handle(SEXP private, int cleanup) { result = PROTECT(R_MakeExternalPtr(handle, private, R_NilValue)); R_RegisterCFinalizerEx(result, processx__finalizer, 1); handle->cleanup = cleanup; + handle->cleanup_grace = cleanup_grace; UNPROTECT(1); return result; @@ -951,8 +952,8 @@ void processx__handle_destroy(processx_handle_t *handle) { SEXP processx_exec(SEXP command, SEXP args, SEXP pty, SEXP pty_options, SEXP connections, SEXP env, SEXP windows_verbatim_args, SEXP windows_hide, SEXP windows_detached_process, - SEXP private, SEXP cleanup, SEXP wd, SEXP encoding, - SEXP tree_id, SEXP linux_pdeathsig) { + SEXP private, SEXP cleanup, SEXP cleanup_grace, SEXP wd, + SEXP encoding, SEXP tree_id, SEXP linux_pdeathsig) { const char *ccommand = CHAR(STRING_ELT(command, 0)); const char *cencoding = CHAR(STRING_ELT(encoding, 0)); @@ -971,6 +972,7 @@ SEXP processx_exec(SEXP command, SEXP args, SEXP pty, SEXP pty_options, processx_handle_t *handle; int ccleanup = INTEGER(cleanup)[0]; + double ccleanup_grace = REAL(cleanup_grace)[0]; SEXP result; DWORD dwerr; @@ -1038,7 +1040,7 @@ SEXP processx_exec(SEXP command, SEXP args, SEXP pty, SEXP pty_options, } } - result = PROTECT(processx__make_handle(private, ccleanup)); + result = PROTECT(processx__make_handle(private, ccleanup, ccleanup_grace)); handle = R_ExternalPtrAddr(result); application_path = processx__search_path(application, cwd, path); diff --git a/tests/testthat/_snaps/Darwin/process.md b/tests/testthat/_snaps/Darwin/process.md index 64fb478b..55153d60 100644 --- a/tests/testthat/_snaps/Darwin/process.md +++ b/tests/testthat/_snaps/Darwin/process.md @@ -6,7 +6,7 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! cannot start processx process '/' (system error 2, No such file or directory) @unix/processx.c:651 (processx_exec) + ! cannot start processx process '/' (system error 2, No such file or directory) @unix/processx.c:650 (processx_exec) # working directory does not exist @@ -16,5 +16,5 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:651 (processx_exec) + ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:650 (processx_exec) diff --git a/tests/testthat/_snaps/Darwin/run.md b/tests/testthat/_snaps/Darwin/run.md index 87736165..d56145a8 100644 --- a/tests/testthat/_snaps/Darwin/run.md +++ b/tests/testthat/_snaps/Darwin/run.md @@ -6,5 +6,5 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:651 (processx_exec) + ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:650 (processx_exec) diff --git a/tests/testthat/_snaps/Linux/process.md b/tests/testthat/_snaps/Linux/process.md index 64fb478b..55153d60 100644 --- a/tests/testthat/_snaps/Linux/process.md +++ b/tests/testthat/_snaps/Linux/process.md @@ -6,7 +6,7 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! cannot start processx process '/' (system error 2, No such file or directory) @unix/processx.c:651 (processx_exec) + ! cannot start processx process '/' (system error 2, No such file or directory) @unix/processx.c:650 (processx_exec) # working directory does not exist @@ -16,5 +16,5 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:651 (processx_exec) + ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:650 (processx_exec) diff --git a/tests/testthat/_snaps/Linux/run.md b/tests/testthat/_snaps/Linux/run.md index 87736165..d56145a8 100644 --- a/tests/testthat/_snaps/Linux/run.md +++ b/tests/testthat/_snaps/Linux/run.md @@ -6,5 +6,5 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:651 (processx_exec) + ! cannot start processx process '/px' (system error 2, No such file or directory) @unix/processx.c:650 (processx_exec) diff --git a/tests/testthat/_snaps/Windows/process.md b/tests/testthat/_snaps/Windows/process.md index ed1e8608..ce3bad80 100644 --- a/tests/testthat/_snaps/Windows/process.md +++ b/tests/testthat/_snaps/Windows/process.md @@ -6,7 +6,7 @@ Error in `process_initialize()`: ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: - ! Command '/' not found @win/processx.c:1059 (processx_exec) + ! Command '/' not found @win/processx.c:1061 (processx_exec) # working directory does not exist @@ -17,5 +17,5 @@ ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: ! create process '/px' (system error 267, The directory name is invalid. - ) @win/processx.c:1370 (processx_exec) + ) @win/processx.c:1372 (processx_exec) diff --git a/tests/testthat/_snaps/Windows/run.md b/tests/testthat/_snaps/Windows/run.md index 5dc6efa8..20c7fd31 100644 --- a/tests/testthat/_snaps/Windows/run.md +++ b/tests/testthat/_snaps/Windows/run.md @@ -7,5 +7,5 @@ ! ! Native call to `processx_exec` failed Caused by error in `chain_call(...)` at initialize.R::: ! create process '/px' (system error 267, The directory name is invalid. - ) @win/processx.c:1370 (processx_exec) + ) @win/processx.c:1372 (processx_exec) diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R index 08475344..6d7db617 100644 --- a/tests/testthat/helper.R +++ b/tests/testthat/helper.R @@ -249,3 +249,4 @@ retry_until <- function(fn, interrupt = 0.2, timeout = 5) { skip_on_cran() stop("timeout") } + diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index 7b2d7ed4..8bf7ab87 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -339,3 +339,105 @@ test_that("get_end_time", { # cached: second call returns the same value expect_equal(p$get_end_time(), et) }) + +test_that("can kill process with grace", { + # https://github.com/r-lib/callr/pull/250 + skip_on_os("windows") + skip_if_not_installed("callr", "3.7.3.9001") + + withr::local_envvar("PROCESSX_R_SIGTERM_CLEANUP" = "true") + + # Write subprocess `tempdir()` to this file + out <- tempfile() + defer(rimraf(out)) + + fn <- function(file) { + file.create(tempfile()) + cat(paste0(tempdir(), "\n"), file = file) + } + get_temp_dir <- function(frame = parent.frame()) { + dir <- readLines(out) + expect_length(dir, 1) + defer(rimraf(dir), frame = frame) + dir + } + + # Check that SIGTERM was called on subprocess by examining side + # effect of tempdir cleanup + p <- callr::r_session$new() + p$run(fn, list(file = out)) + dir <- get_temp_dir() + p$kill(grace = 0.1) + retry_until(function() !dir.exists(dir)) + + # When `grace` is 0, the tempdir isn't cleaned up + p <- callr::r_session$new() + p$run(fn, list(file = out)) + dir <- get_temp_dir() + p$kill(grace = 0) + expect_true(dir.exists(dir)) +}) + +test_that("can use custom `cleanup_signal`", { + # https://github.com/r-lib/callr/pull/250 + skip_if_not_installed("callr", "3.7.4") + + withr::local_envvar("PROCESSX_R_SIGTERM_CLEANUP" = "true") + + # Should become the default in callr + opts <- callr::r_process_options( + extra = list( + cleanup_grace = 0.1 + ) + ) + p <- callr::r_session$new(opts) + + out <- tempfile() + defer(rimraf(out)) + + fn <- function(file) { + file.create(tempfile()) + writeLines(tempdir(), file) + } + p$run(fn, list(file = out)) + + dir <- readLines(out) + defer(rimraf(dir)) + + # Needs POSIX signals + skip_on_os("windows") + + # GC `p` to trigger finalizer; R doesn't guarantee finalizers run on a + # single gc(), so we retry until the side-effect is observed + rm(p) + retry_until(function() { + gc() + !dir.exists(dir) + }) +}) + +test_that("process survives SIGTERM when ignored", { + skip_on_os("windows") + px <- get_tool("px") + p <- process$new(px, c("sigterm", "ignore", "outln", "ready", "sleep", "10"), + stdout = "|") + defer(p$kill()) + p$poll_io(1000) + expect_equal(p$read_output_lines(), "ready") + tools::pskill(p$get_pid(), tools::SIGTERM) + tools::pskill(p$get_pid(), tools::SIGTERM) + expect_true(p$is_alive()) +}) + +test_that("can kill with SIGTERM when ignored", { + skip_on_os("windows") + px <- get_tool("px") + p <- process$new(px, c("sigterm", "ignore", "outln", "ready", "sleep", "10"), + stdout = "|") + defer(p$kill()) + p$poll_io(1000) + p$read_output_lines() + p$signal(tools::SIGTERM) + Sys.sleep(0.05) + expect_true(p$is_alive()) +})