Remove legacy non-unwind protect code paths

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,

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.

src/callback_registry.cpp

@@ -8,162 +8,6 @@
 
 static std::atomic<uint64_t> 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
 // ============================================================================

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<void (void)> 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;

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);