From 2a5ca8e46a0cd81af64197171f06a1f3b2138bef Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 19 Apr 2023 10:45:11 +0200 Subject: [PATCH 1/6] Set up cleanup process on init to avoid signal-safety issues --- src/client.c | 18 +++++++++++++++--- tests/testthat/test-process.R | 4 ++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/client.c b/src/client.c index ad9db57e..e91c7229 100644 --- a/src/client.c +++ b/src/client.c @@ -240,10 +240,18 @@ SEXP processx_base64_decode(SEXP array); #include #include +FILE *cleanup_file; +int cleanup_fd; + void term_handler(int n) { - // Need the cast and the +1 to ignore compiler warning about unused - // return value. - (void) (system("rm -rf \"$R_SESSION_TMPDIR\"") + 1); + // `fwrite()` is not async-safe + write(cleanup_fd, "\n", 1); + + // `pclose()` is not async-safe. Just assume that the cleanup + // process is going to terminate naturally once it's finished the + // command. The file descriptors will be closed automatically on + // exit. This also allows us to exit faster. + // Continue signal raise(SIGTERM); } @@ -253,6 +261,10 @@ void install_term_handler(void) { return; } + // FIXME: Is it a bit dangerous to use an envvar here? + cleanup_file = popen("read input && [ \"$input\" = \"\" ] && rm -rf \"$R_SESSION_TMPDIR\"", "w"); + cleanup_fd = fileno(cleanup_file); + struct sigaction sig = {{ 0 }}; sig.sa_handler = term_handler; sig.sa_flags = SA_RESETHAND; diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index ead69473..00cf8b04 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -93,6 +93,10 @@ test_that("R process is installed with a SIGTERM cleanup handler", { p$signal(ps::signals()$SIGTERM) p$wait() + + # We're no longer waiting for the cleanup process to finish so give + # some breathing room + Sys.sleep(0.2) expect_false(dir.exists(p_temp_dir)) # Disabled case From e25c5ffbce5c141e3630a8a94aecf85ed34d08ee Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 19 Apr 2023 11:50:29 +0200 Subject: [PATCH 2/6] Clean up process on quit --- src/client.c | 39 ++++++++++++++++++++++++++++++++--- tests/testthat/test-process.R | 15 ++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/client.c b/src/client.c index e91c7229..0278f644 100644 --- a/src/client.c +++ b/src/client.c @@ -240,9 +240,23 @@ SEXP processx_base64_decode(SEXP array); #include #include -FILE *cleanup_file; -int cleanup_fd; - +static FILE *cleanup_file; +static int cleanup_fd; +static int needs_handler_cleanup = 0; + +// We want to clean up the temporary directory on SIGTERM. Since we +// can barely do anything safely from a signal handler, we set up a +// process ahead of time that is going to run `rm -rf tempdir` on +// termination. The process is waiting for an empty line that we send +// with the signal-async-safe function `write()`. +// +// On any other input, the process quits without removing the +// tempdir. This is used from the normal-quit handler (the +// `R_unload_client()` function that is called by `dyn.load()` which +// callr set up to be called on quit) to terminate the process while +// leaving the directory intact. + +static void term_handler(int n) { // `fwrite()` is not async-safe write(cleanup_fd, "\n", 1); @@ -264,6 +278,7 @@ void install_term_handler(void) { // FIXME: Is it a bit dangerous to use an envvar here? cleanup_file = popen("read input && [ \"$input\" = \"\" ] && rm -rf \"$R_SESSION_TMPDIR\"", "w"); cleanup_fd = fileno(cleanup_file); + needs_handler_cleanup = 1; struct sigaction sig = {{ 0 }}; sig.sa_handler = term_handler; @@ -271,6 +286,8 @@ void install_term_handler(void) { sigaction(SIGTERM, &sig, NULL); } +void R_unload_client(DllInfo *_dll); + #endif // not _WIN32 @@ -283,6 +300,9 @@ static const R_CallMethodDef callMethods[] = { { "processx_set_stderr", (DL_FUNC) &processx_set_stderr, 2 }, { "processx_set_stdout_to_file", (DL_FUNC) &processx_set_stdout_to_file, 1 }, { "processx_set_stderr_to_file", (DL_FUNC) &processx_set_stderr_to_file, 1 }, +#ifndef _WIN32 + { "R_unload_client", (DL_FUNC) &R_unload_client, 1 }, +#endif { NULL, NULL, 0 } }; @@ -295,3 +315,16 @@ void R_init_client(DllInfo *dll) { install_term_handler(); #endif } + +#ifndef _WIN32 +// Also called on session quit by callr +void R_unload_client(DllInfo *_dll) { + if (needs_handler_cleanup) { + // Close cleanup process without removing the tempdir in case it's + // still needed + const char* quit = "quit\n"; + fwrite(quit, strlen(quit), 1, cleanup_file); + pclose(cleanup_file); + } +} +#endif diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index 00cf8b04..da02aa30 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -75,6 +75,8 @@ test_that("R process is installed with a SIGTERM cleanup handler", { # Needs POSIX signal handling skip_on_os("windows") + n_children <- length(ps::ps_children()) + # Enabled case withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = "true")) @@ -86,11 +88,15 @@ test_that("R process is installed with a SIGTERM cleanup handler", { } p <- callr::r_session$new() + h <- ps::ps_handle(p$get_pid()) p$run(fn, list(file = out)) p_temp_dir <- readLines(out) expect_true(dir.exists(p_temp_dir)) + # The cleanup process has been launched + expect_length(ps::ps_children(), n_children + 1) + p$signal(ps::signals()$SIGTERM) p$wait() @@ -98,6 +104,15 @@ test_that("R process is installed with a SIGTERM cleanup handler", { # some breathing room Sys.sleep(0.2) expect_false(dir.exists(p_temp_dir)) + expect_length(ps::ps_children(), n_children) + + # The cleanup process is terminated on quit + p <- callr::r_session$new() + h <- ps::ps_handle(p$get_pid()) + + expect_length(ps::ps_children(), n_children + 1) + p$run(function() quit("no")) + expect_length(ps::ps_children(), n_children) # Disabled case withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = NA_character_)) From 1b93d22fd2631d9d18d31d9ef9705040e06b1d25 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 25 Apr 2023 18:21:36 +0200 Subject: [PATCH 3/6] Ignore SIGTERM in cleanup processes --- R/utils.R | 5 +++ src/client.c | 10 +++++ tests/testthat/helper.R | 16 ++++++++ tests/testthat/test-process.R | 72 +++++++++++++++++++++++++++++++++-- 4 files changed, 99 insertions(+), 4 deletions(-) diff --git a/R/utils.R b/R/utils.R index 10da64eb..5a78f802 100644 --- a/R/utils.R +++ b/R/utils.R @@ -295,3 +295,8 @@ ends_with <- function(x, post) { l <- nchar(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) +} diff --git a/src/client.c b/src/client.c index 0278f644..4dee6e91 100644 --- a/src/client.c +++ b/src/client.c @@ -275,6 +275,14 @@ void install_term_handler(void) { return; } + // Ignore SIGTERM in cleanup process via inherited procmask + sigset_t mask; + sigemptyset(&mask); + sigaddset(&mask, SIGTERM); + + sigset_t old; + sigprocmask(SIG_BLOCK, &mask, &old); + // FIXME: Is it a bit dangerous to use an envvar here? cleanup_file = popen("read input && [ \"$input\" = \"\" ] && rm -rf \"$R_SESSION_TMPDIR\"", "w"); cleanup_fd = fileno(cleanup_file); @@ -284,6 +292,8 @@ void install_term_handler(void) { sig.sa_handler = term_handler; sig.sa_flags = SA_RESETHAND; sigaction(SIGTERM, &sig, NULL); + + sigprocmask(SIG_SETMASK, &old, NULL); } void R_unload_client(DllInfo *_dll); diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R index 93caae0f..e541087c 100644 --- a/tests/testthat/helper.R +++ b/tests/testthat/helper.R @@ -150,3 +150,19 @@ scrub_srcref <- function(x) { } err$register_testthat_print() + +poll_until <- function(fn, interrupt = 0.2, timeout = 5) { + time <- Sys.time() + timeout <- time + timeout + + while (Sys.time() < timeout) { + if (fn()) { + expect_true(TRUE) + return() + } + Sys.sleep(interrupt) + } + + skip_on_cran() + stop("timeout") +} diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index da02aa30..ffd4fcc2 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -81,6 +81,7 @@ test_that("R process is installed with a SIGTERM cleanup handler", { withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = "true")) out <- tempfile() + defer(unlink(out, TRUE, TRUE)) fn <- function(file) { file.create(tempfile()) @@ -100,10 +101,9 @@ test_that("R process is installed with a SIGTERM cleanup handler", { p$signal(ps::signals()$SIGTERM) p$wait() - # We're no longer waiting for the cleanup process to finish so give - # some breathing room - Sys.sleep(0.2) - expect_false(dir.exists(p_temp_dir)) + # We're no longer waiting for the cleanup process to finish so poll + # until finished + poll_until(function() !dir.exists(p_temp_dir)) expect_length(ps::ps_children(), n_children) # The cleanup process is terminated on quit @@ -132,3 +132,67 @@ test_that("R process is installed with a SIGTERM cleanup handler", { # Was not cleaned up expect_true(dir.exists(p_temp_dir)) }) + +test_that("can kill process tree with SIGTERM", { + # https://github.com/r-lib/callr/pull/250 + skip_if_not_installed("callr", "3.7.3.9001") + + # Needs POSIX signal handling + skip_on_os("windows") + + withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = "true")) + + out <- tempfile() + defer(unlink(out, TRUE, TRUE)) + file.create(out) + + fn <- function(recurse, local, file) { + p <- NULL + + if (recurse) { + p <- callr::r_session$new() + p$call( + sys.function(), + list(recurse - 1, local = FALSE, file = file) + ) + } + + if (!local) { + file.create(tempfile()) + cat(paste0(tempdir(), "\n"), file = file, append = TRUE) + + # Sleeping prevents the process to receive an EOF in + # `R_ReadConsole()` (which causes it to quit normally) + Sys.sleep(60) + } + + p + } + + N <- 5 + p <- fn(N, local = TRUE, file = out) + + pid <- p$get_pid() + id <- p$.__enclos_env__$private$tree_id + + temp_dirs <- NULL + + poll_until(function() { + temp_dirs <<- readLines(out) + length(temp_dirs) == N + }) + + ps <- ps::ps_find_tree(id) + + # Kill all ps-marked subprocesses, including the cleanup + # processes. These ignore SIGTERM but should exit quickly when their + # parent process is terminated. + for (p in ps) { + tools::pskill(ps::ps_pid(p)) + } + poll_until(function() { + !any(sapply(ps, function(p) ps::ps_is_running(p))) + }) + + expect_false(any(dir.exists(temp_dirs))) +}) From a9a55bba6b8d5dfcd47e146bdc008509d5c22862 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 25 Apr 2023 18:37:37 +0200 Subject: [PATCH 4/6] Add test for sigkilled parent process --- tests/testthat/test-process.R | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index ffd4fcc2..add26148 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -196,3 +196,24 @@ test_that("can kill process tree with SIGTERM", { expect_false(any(dir.exists(temp_dirs))) }) + +test_that("can sigkill parent of cleanup process", { + # https://github.com/r-lib/callr/pull/250 + skip_if_not_installed("callr", "3.7.3.9001") + + # Needs POSIX signal handling + skip_on_os("windows") + + withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = "true")) + + p <- callr::r_session$new() + p_handle <- ps::ps_handle(p$get_pid()) + + ps <- ps::ps_children(p_handle) + expect_length(ps, 1) + cleanup_p <- ps[[1]] + + # The cleanup process gets an EOF on its stdin and exits + tools::pskill(p$get_pid(), tools::SIGKILL) + poll_until(function() !ps::ps_is_running(cleanup_p)) +}) From 5c3555132ba3332963d0d852d8d915b770e96fc7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 25 Apr 2023 18:48:00 +0200 Subject: [PATCH 5/6] Don't need to exit cleanup process on unload --- src/client.c | 24 ------------------------ tests/testthat/test-process.R | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/client.c b/src/client.c index 4dee6e91..9ab543cb 100644 --- a/src/client.c +++ b/src/client.c @@ -249,12 +249,6 @@ static int needs_handler_cleanup = 0; // process ahead of time that is going to run `rm -rf tempdir` on // termination. The process is waiting for an empty line that we send // with the signal-async-safe function `write()`. -// -// On any other input, the process quits without removing the -// tempdir. This is used from the normal-quit handler (the -// `R_unload_client()` function that is called by `dyn.load()` which -// callr set up to be called on quit) to terminate the process while -// leaving the directory intact. static void term_handler(int n) { @@ -296,8 +290,6 @@ void install_term_handler(void) { sigprocmask(SIG_SETMASK, &old, NULL); } -void R_unload_client(DllInfo *_dll); - #endif // not _WIN32 @@ -310,9 +302,6 @@ static const R_CallMethodDef callMethods[] = { { "processx_set_stderr", (DL_FUNC) &processx_set_stderr, 2 }, { "processx_set_stdout_to_file", (DL_FUNC) &processx_set_stdout_to_file, 1 }, { "processx_set_stderr_to_file", (DL_FUNC) &processx_set_stderr_to_file, 1 }, -#ifndef _WIN32 - { "R_unload_client", (DL_FUNC) &R_unload_client, 1 }, -#endif { NULL, NULL, 0 } }; @@ -325,16 +314,3 @@ void R_init_client(DllInfo *dll) { install_term_handler(); #endif } - -#ifndef _WIN32 -// Also called on session quit by callr -void R_unload_client(DllInfo *_dll) { - if (needs_handler_cleanup) { - // Close cleanup process without removing the tempdir in case it's - // still needed - const char* quit = "quit\n"; - fwrite(quit, strlen(quit), 1, cleanup_file); - pclose(cleanup_file); - } -} -#endif diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index add26148..911ab483 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -197,7 +197,7 @@ test_that("can kill process tree with SIGTERM", { expect_false(any(dir.exists(temp_dirs))) }) -test_that("can sigkill parent of cleanup process", { +test_that("can exit or sigkill parent of cleanup process", { # https://github.com/r-lib/callr/pull/250 skip_if_not_installed("callr", "3.7.3.9001") @@ -213,7 +213,18 @@ test_that("can sigkill parent of cleanup process", { expect_length(ps, 1) cleanup_p <- ps[[1]] - # The cleanup process gets an EOF on its stdin and exits + # Normal exit: The cleanup process gets an EOF on its stdin and exits + p$close() + poll_until(function() !ps::ps_is_running(cleanup_p)) + + p <- callr::r_session$new() + p_handle <- ps::ps_handle(p$get_pid()) + + ps <- ps::ps_children(p_handle) + expect_length(ps, 1) + cleanup_p <- ps[[1]] + + # SIGKILL: Also gets an EOF tools::pskill(p$get_pid(), tools::SIGKILL) poll_until(function() !ps::ps_is_running(cleanup_p)) }) From 7b871edf4cf594d61d4d15eb83bfa1a7f051ad75 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 25 Apr 2023 19:02:04 +0200 Subject: [PATCH 6/6] Fix `-Wunused-result` warning --- src/client.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/client.c b/src/client.c index 9ab543cb..dcf36c4f 100644 --- a/src/client.c +++ b/src/client.c @@ -252,8 +252,9 @@ static int needs_handler_cleanup = 0; static void term_handler(int n) { - // `fwrite()` is not async-safe - write(cleanup_fd, "\n", 1); + // `fwrite()` is not async-safe. Need the cast to avoid + // `-Wunused-result` warning + (void) (write(cleanup_fd, "\n", 1) + 1); // `pclose()` is not async-safe. Just assume that the cleanup // process is going to terminate naturally once it's finished the