diff --git a/DESCRIPTION b/DESCRIPTION index a0516e2..b7933bb 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -23,9 +23,9 @@ License: MIT + file LICENSE URL: https://later.r-lib.org, https://github.com/r-lib/later BugReports: https://github.com/r-lib/later/issues Depends: - R (>= 3.4) + R (>= 3.5) Imports: - Rcpp (>= 0.12.9), + Rcpp (>= 1.0.10), rlang Suggests: knitr, diff --git a/NEWS.md b/NEWS.md index 5611e19..ee3c883 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # later (development version) +* Requires R >= 3.5 and Rcpp >= 1.0.10 to ensure later callback errors are properly handled. We have removed the fallback to legacy pre-later 1.4.0 behaviour (#241). + # later 1.4.4 * Fixed timings in a test (#237). No user-facing changes. diff --git a/src/callback_registry.cpp b/src/callback_registry.cpp index 1ab2ad5..74fe501 100644 --- a/src/callback_registry.cpp +++ b/src/callback_registry.cpp @@ -8,162 +8,6 @@ static std::atomic nextCallbackId(1); -// ============================================================================ -// Invoke functions -// ============================================================================ - -enum InvokeResult { - INVOKE_IN_PROGRESS, - INVOKE_INTERRUPTED, - INVOKE_ERROR, - INVOKE_CPP_ERROR, - INVOKE_COMPLETED -}; - -// This is set by invoke_c(). I -static InvokeResult last_invoke_result; -static std::string last_invoke_message; - -// A wrapper for calling R_CheckUserInterrupt via R_ToplevelExec. -void checkInterruptFn(void*) { - R_CheckUserInterrupt(); -} - -// The purpose of this function is to provide a plain C function to be called -// by R_ToplevelExec. Because it's called as a C function, it must not throw -// exceptions. Because this function returns void, the way for it to report -// the result to its caller is by setting last_invoke_result. -// -// This code needs to be able to handle interrupts, R errors, and C++ -// exceptions. There are many ways these things can happen. -// -// * If the Callback object is a RcppFunctionCallback, then in the case of an -// interrupt or an R error, it will throw a C++ exception. These exceptions -// are the ones defined by Rcpp, and they will be caught by the try-catch in -// this function. -// * It could be a StdFunctionCallback with C or C++ code. -// * If the function invokes an Rcpp::Function and an interrupt or R error -// happens within the Rcpp::Function, it will throw exceptions just like -// the RcppFunctionCallback case, and they will be caught. -// * If some other C++ exception occurs, it will be caught. -// * If an interrupt (Ctrl-C, or Esc in RStudio) is received (outside of an -// Rcpp::Function), this function will continue through to the end (and -// set the state to INVOKE_COMPLETED). Later, when the invoke_wrapper() -// function (which called this one) checks to see if the interrupt -// happened, it will set the state to INVOKE_INTERRUPTED. (Note that it is -// potentially possible for an interrupt and an exception to occur, in -// which case we set the state to INVOKE_ERROR.) -// * If the function calls R code with Rf_eval(), an interrupt or R error -// could occur. If it's an interrupt, then it will be detect as in the -// previous case. If an error occurs, then that error will be detected by -// the invoke_wrapper() function (which called this one) and the state -// will be set to INVOKE_ERROR. -// -// Note that the last case has one potentially problematic issue. If an error -// occurs in R code, then it will longjmp out of of this function, back to its -// caller, invoke_wrapped(). This will longjmp out of a try statement, which -// is generally not a good idea. We don't know ahead of time whether the -// Callback may longjmp or throw an exception -- some Callbacks could -// potentially do both. -// -// The alternative is to move the try-catch out of this function and into -// invoke_wrapped(), surrounding the `R_ToplevelExec(invoke_c, ...)`. However, -// if we do this, then exceptions would pass through the R_ToplevelExec, which -// is dangerous because it is plain C code. The current way of doing it is -// imperfect, but less dangerous. -// -// There does not seem to be a 100% safe way to call functions which could -// either longjmp or throw exceptions. If we do figure out a way to do that, -// it should be used here. -extern "C" void invoke_c(void* callback_p) { - ASSERT_MAIN_THREAD() - last_invoke_result = INVOKE_IN_PROGRESS; - last_invoke_message = ""; - - Callback* cb_p = (Callback*)callback_p; - - try { - cb_p->invoke(); - } - catch(Rcpp::internal::InterruptedException &e) { - // Reaches here if the callback is in Rcpp code and an interrupt occurs. - DEBUG_LOG("invoke_c: caught Rcpp::internal::InterruptedException", LOG_INFO); - last_invoke_result = INVOKE_INTERRUPTED; - return; - } - catch(Rcpp::eval_error &e) { - // Reaches here if an R-level error happens in an Rcpp::Function. - DEBUG_LOG("invoke_c: caught Rcpp::eval_error", LOG_INFO); - last_invoke_result = INVOKE_ERROR; - last_invoke_message = e.what(); - return; - } - catch(Rcpp::exception& e) { - // Reaches here if an R-level error happens in an Rcpp::Function. - DEBUG_LOG("invoke_c: caught Rcpp::exception", LOG_INFO); - last_invoke_result = INVOKE_ERROR; - last_invoke_message = e.what(); - return; - } - catch(std::exception& e) { - // Reaches here if some other (non-Rcpp) C++ exception is thrown. - DEBUG_LOG(std::string("invoke_c: caught std::exception: ") + typeid(e).name(), - LOG_INFO); - last_invoke_result = INVOKE_CPP_ERROR; - last_invoke_message = e.what(); - return; - } - catch( ... ) { - // Reaches here if a non-exception C++ object is thrown. - DEBUG_LOG(std::string("invoke_c: caught unknown object: ") + typeid(std::current_exception()).name(), - LOG_INFO); - last_invoke_result = INVOKE_CPP_ERROR; - return; - } - - // Reaches here if no exceptions are thrown. It's possible to get here if an - // interrupt was received outside of Rcpp code, or if an R error happened - // using Rf_eval(). - DEBUG_LOG("invoke_c: COMPLETED", LOG_DEBUG); - last_invoke_result = INVOKE_COMPLETED; -} - -// Wrapper method for invoking a callback. The Callback object has an invoke() -// method, but instead of invoking it directly, this method should be used -// instead. The purpose of this method is to call invoke(), but wrap it in a -// R_ToplevelExec, so that any LONGJMPs (due to errors in R functions) won't -// cross that barrier in the call stack. If interrupts, exceptions, or -// LONGJMPs do occur, this function throws a C++ exception. -void Callback::invoke_wrapped() const { - ASSERT_MAIN_THREAD() - Rboolean result = R_ToplevelExec(invoke_c, (void*)this); - - if (!result) { - DEBUG_LOG("invoke_wrapped: R_ToplevelExec return is FALSE; error or interrupt occurred in R code", LOG_INFO); - last_invoke_result = INVOKE_ERROR; - } - - if (R_ToplevelExec(checkInterruptFn, NULL) == FALSE) { - // Reaches here if the callback is C/C++ code and an interrupt occurs. - DEBUG_LOG("invoke_wrapped: interrupt (outside of R code) detected by R_CheckUserInterrupt", LOG_INFO); - last_invoke_result = INVOKE_INTERRUPTED; - } - - switch (last_invoke_result) { - case INVOKE_INTERRUPTED: - DEBUG_LOG("invoke_wrapped: throwing Rcpp::internal::InterruptedException", LOG_INFO); - throw Rcpp::internal::InterruptedException(); - case INVOKE_ERROR: - DEBUG_LOG("invoke_wrapped: throwing Rcpp::exception", LOG_INFO); - throw Rcpp::exception(last_invoke_message.c_str()); - case INVOKE_CPP_ERROR: - throw std::runtime_error("invoke_wrapped: throwing std::runtime_error"); - default: - return; - } -} - - // ============================================================================ // StdFunctionCallback // ============================================================================ diff --git a/src/callback_registry.h b/src/callback_registry.h index 90a203a..4100ad5 100644 --- a/src/callback_registry.h +++ b/src/callback_registry.h @@ -39,8 +39,6 @@ class Callback { virtual void invoke() const = 0; - void invoke_wrapped() const; - virtual Rcpp::RObject rRepresentation() const = 0; Timestamp when; @@ -57,15 +55,12 @@ class StdFunctionCallback : public Callback { StdFunctionCallback(Timestamp when, std::function func); void invoke() const { -#ifdef RCPP_USING_UNWIND_PROTECT // See https://github.com/r-lib/later/issues/191 + // See https://github.com/r-lib/later/issues/191 and https://github.com/r-lib/later/pull/241 Rcpp::unwindProtect([this]() { BEGIN_RCPP func(); END_RCPP }); -#else - func(); -#endif } Rcpp::RObject rRepresentation() const; diff --git a/src/later.cpp b/src/later.cpp index 74747c7..2922b60 100644 --- a/src/later.cpp +++ b/src/later.cpp @@ -205,13 +205,8 @@ bool execCallbacksOne( break; } -#ifdef RCPP_USING_UNWIND_PROTECT // See https://github.com/r-lib/later/issues/191 // This line may throw errors! callback->invoke(); -#else - // This line may throw errors! - callback->invoke_wrapped(); -#endif } while (runAll);