Skip to content

Commit

Permalink
Fix error/interrupt handling logic to work with Rcpp 1.0.10+
Browse files Browse the repository at this point in the history
The introduction of unwind protection in Rcpp required us to stop
working so hard to catch and re-propagate different kinds of errors
across the R->C++->R boundary. This went unnoticed for so long
because the interrupt-related tests were already misbehaving on
CI and on CRAN. I've now refactored the tests so that the test
skipping can be narrowly scoped to just the interrupts, and even
those, I'd like to try again on CI.
  • Loading branch information
jcheng5 committed Oct 25, 2024
1 parent 968542a commit 2922839
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 36 deletions.
7 changes: 7 additions & 0 deletions src/later.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,15 @@ bool execCallbacksOne(
if (callbacks.size() == 0) {
break;
}

#ifdef RCPP_USING_UNWIND_PROTECT // See https://github.com/r-lib/later/issues/191
// This line may throw errors!
callbacks[0]->invoke();
#else
// This line may throw errors!
callbacks[0]->invoke_wrapped();
#endif

} while (runAll);

// I think there's no need to lock this since it's only modified from the
Expand Down
78 changes: 42 additions & 36 deletions tests/testthat/test-run_now.R
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,11 @@ test_that("Callbacks cannot affect the caller", {
run_now(1)
return(200)
}
expect_error(f())
# jcheng 2024-10-24: Apparently this works now, maybe because having
# RCPP_USING_UNWIND_PROTECT means we don't need to use R_ToplevelExec to call
# callbacks?
# expect_error(f())
expect_identical(f(), 100)


# In this case, f() should return normally, and then when g() causes later to
Expand All @@ -150,11 +154,6 @@ test_that("Callbacks cannot affect the caller", {


test_that("interrupt and exception handling", {
# These tests may fail in automated test environments due to the way they
# handle interrupts. (See #102)
skip_on_ci()
skip_on_cran()

# =======================================================
# Errors and interrupts in R callbacks
# =======================================================
Expand All @@ -172,21 +171,6 @@ test_that("interrupt and exception handling", {
)
expect_true(grepl("oopsie", error_obj$message))

# interrupt
interrupted <- FALSE
tryCatch(
{
later(function() { tools::pskill(Sys.getpid(), tools::SIGINT) })
run_now()
},
interrupt = function(e) {
interrupted <<- TRUE
}
)
expect_true(interrupted)




# =======================================================
# Exceptions in C++ callbacks
Expand Down Expand Up @@ -221,11 +205,11 @@ test_that("interrupt and exception handling", {
} else if (value == 3) {
// Send an interrupt to the process.
kill(getpid(), SIGINT);
sleep(3);
R_CheckUserInterrupt();
} else if (value == 4) {
// Calls R function via Rcpp, which sends interrupt signal and then
// sleeps. Note: This gets converted to std::runtime_error.
// sleeps.
Function("r_sleep_interrupt")();
} else if (value == 5) {
Expand Down Expand Up @@ -282,24 +266,46 @@ test_that("interrupt and exception handling", {
)
expect_true(errored)

interrupted <- FALSE
tryCatch(
{ cpp_error(3); run_now() },
interrupt = function(e) interrupted <<- TRUE
)
expect_true(interrupted)

errored <- FALSE
tryCatch(
{ cpp_error(4); run_now() },
interrupt = function(e) interrupted <<- TRUE
)
expect_true(interrupted)

errored <- FALSE
tryCatch(
{ cpp_error(5); run_now() },
error = function(e) errored <<- TRUE
)
expect_true(errored)

test_that("Interrupts work", {
# These tests may fail in automated test environments due to the way they
# handle interrupts. (See #102)
# jcheng 2024-10-24: Let's find out if this is still true
# skip_on_ci()
skip_on_cran()

# interrupt
interrupted <- FALSE
tryCatch(
{
later(function() { tools::pskill(Sys.getpid(), tools::SIGINT); Sys.sleep(100) })
run_now()
},
interrupt = function(e) {
interrupted <<- TRUE
}
)
expect_true(interrupted)

interrupted <- FALSE
tryCatch(
{ cpp_error(3); run_now() },
interrupt = function(e) interrupted <<- TRUE
)
expect_true(interrupted)

interrupted <- FALSE
tryCatch(
{ cpp_error(4); run_now() },
interrupt = function(e) interrupted <<- TRUE
)
expect_true(interrupted)
})
})

0 comments on commit 2922839

Please sign in to comment.