From 2a5ca8e46a0cd81af64197171f06a1f3b2138bef Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 19 Apr 2023 10:45:11 +0200 Subject: [PATCH 01/10] 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 02/10] 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 03/10] 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 04/10] 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 05/10] 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 06/10] 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 From 1cc4b8eb17d0e95e8ee7d59b4c7df5fdf5f30044 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Cs=C3=A1rdi?= Date: Thu, 23 Apr 2026 10:10:49 +0200 Subject: [PATCH 07/10] Use fork()+execv() in SIGTERM handler instead of a pre-forked process Replace the popen()-based approach with fork()+execv("/bin/rm") called directly from the signal handler. Both are async-signal-safe, and this avoids the persistent child process per R session as well as the envvar-expansion-at-runtime concern (R_SESSION_TMPDIR is now captured into a static buffer at handler installation time). Drop tests that verified the persistent child process lifecycle, and add a poll_until() in the process-tree test to account for the rm child now being spawned at signal time rather than pre-started. --- src/client.c | 51 +++++++++++---------------------- tests/testthat/test-process.R | 53 ++--------------------------------- 2 files changed, 19 insertions(+), 85 deletions(-) diff --git a/src/client.c b/src/client.c index 8dad6560..8eb159d9 100644 --- a/src/client.c +++ b/src/client.c @@ -251,56 +251,39 @@ SEXP processx_base64_decode(SEXP array); #include #include +#include +#include -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()`. - -static -void term_handler(int n) { - // `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 - // command. The file descriptors will be closed automatically on - // exit. This also allows us to exit faster. +static char tmpdir_buf[PATH_MAX]; +static char *rm_argv[] = { "/bin/rm", "-rf", tmpdir_buf, NULL }; +static void term_handler(int n) { + pid_t pid = fork(); + if (pid == 0) { + execv("/bin/rm", rm_argv); + _exit(127); + } // Continue signal raise(SIGTERM); } void install_term_handler(void) { - if (! getenv("PROCESSX_R_SIGTERM_CLEANUP")) { + if (!getenv("PROCESSX_R_SIGTERM_CLEANUP")) { 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); + const char *tmpdir = getenv("R_SESSION_TMPDIR"); + if (!tmpdir) { + 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); - needs_handler_cleanup = 1; + // Capture the path now so the signal handler needs no getenv() + snprintf(tmpdir_buf, sizeof(tmpdir_buf), "%s", tmpdir); struct sigaction sig = {{ 0 }}; sig.sa_handler = term_handler; sig.sa_flags = SA_RESETHAND; sigaction(SIGTERM, &sig, NULL); - - sigprocmask(SIG_SETMASK, &old, NULL); } #endif // not _WIN32 diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index 799d43da..3eb3e0e0 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -93,8 +93,6 @@ 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")) @@ -107,30 +105,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() - # 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 - 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_)) @@ -202,9 +185,6 @@ test_that("can kill process tree with SIGTERM", { 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)) } @@ -212,40 +192,11 @@ test_that("can kill process tree with SIGTERM", { !any(sapply(ps, function(p) ps::ps_is_running(p))) }) + # rm -rf runs in a forked child; poll until it finishes + poll_until(function() !any(dir.exists(temp_dirs))) expect_false(any(dir.exists(temp_dirs))) }) -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") - - # 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]] - - # 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)) -}) test_that("linux_pdeathsig kills child when parent exits", { skip_if(!is_linux()) From 7ee77192f2a1f8da567cde06b29c2c94619750f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Cs=C3=A1rdi?= Date: Thu, 23 Apr 2026 10:28:16 +0200 Subject: [PATCH 08/10] Skip some tests in ASAN/UBSAN/valgrind Seem hard to make them work. --- tests/testthat/test-process.R | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index 3eb3e0e0..798ad6ba 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -141,6 +141,12 @@ test_that("can kill process tree with SIGTERM", { # Needs POSIX signal handling skip_on_os("windows") + # fork() in signal handler can deadlock under ASAN; shutdown is too slow + # for the poll timeout under UBSAN and valgrind + skip_if(is_asan()) + skip_if(is_ubsan()) + skip_if(is_valgrind()) + withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = "true")) out <- tempfile() From a2beec214c5b158092436d15b48d8807809e3b3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Cs=C3=A1rdi?= Date: Thu, 23 Apr 2026 10:30:43 +0200 Subject: [PATCH 09/10] Use withr::defer in tests Instead of reimplementing it. --- R/utils.R | 4 ---- tests/testthat/test-process.R | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/R/utils.R b/R/utils.R index 612a801f..4b4b215e 100644 --- a/R/utils.R +++ b/R/utils.R @@ -352,7 +352,3 @@ 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) -} diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index 798ad6ba..c40f34fa 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -97,7 +97,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)) + withr::defer(unlink(out, TRUE, TRUE)) fn <- function(file) { file.create(tempfile()) @@ -150,7 +150,7 @@ test_that("can kill process tree with SIGTERM", { withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = "true")) out <- tempfile() - defer(unlink(out, TRUE, TRUE)) + withr::defer(unlink(out, TRUE, TRUE)) file.create(out) fn <- function(recurse, local, file) { From 3962cf428c64927cebe3df77fd35a6186984451a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Cs=C3=A1rdi?= Date: Thu, 23 Apr 2026 10:32:31 +0200 Subject: [PATCH 10/10] Better helper function name: poll_until -> retry_until 'poll' refers to the Unix system call in the processx codebase. --- tests/testthat/helper.R | 2 +- tests/testthat/test-process.R | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R index 9c6a4530..08475344 100644 --- a/tests/testthat/helper.R +++ b/tests/testthat/helper.R @@ -234,7 +234,7 @@ get_deadline <- function(secs = 1, asan_secs = secs * 100) { err$register_testthat_print() -poll_until <- function(fn, interrupt = 0.2, timeout = 5) { +retry_until <- function(fn, interrupt = 0.2, timeout = 5) { time <- Sys.time() timeout <- time + timeout diff --git a/tests/testthat/test-process.R b/tests/testthat/test-process.R index c40f34fa..7b2d7ed4 100644 --- a/tests/testthat/test-process.R +++ b/tests/testthat/test-process.R @@ -113,7 +113,7 @@ test_that("R process is installed with a SIGTERM cleanup handler", { p$signal(ps::signals()$SIGTERM) p$wait() - poll_until(function() !dir.exists(p_temp_dir)) + retry_until(function() !dir.exists(p_temp_dir)) # Disabled case withr::local_envvar(c(PROCESSX_R_SIGTERM_CLEANUP = NA_character_)) @@ -184,7 +184,7 @@ test_that("can kill process tree with SIGTERM", { temp_dirs <- NULL - poll_until(function() { + retry_until(function() { temp_dirs <<- readLines(out) length(temp_dirs) == N }) @@ -194,12 +194,12 @@ test_that("can kill process tree with SIGTERM", { for (p in ps) { tools::pskill(ps::ps_pid(p)) } - poll_until(function() { + retry_until(function() { !any(sapply(ps, function(p) ps::ps_is_running(p))) }) # rm -rf runs in a forked child; poll until it finishes - poll_until(function() !any(dir.exists(temp_dirs))) + retry_until(function() !any(dir.exists(temp_dirs))) expect_false(any(dir.exists(temp_dirs))) })