Remove cpp11_should_unwind_protect altogether (#327)

* Revert #299

* Remove unwind protect global variable altogether

* Add a descriptive test

* Add FAQ bullets about `unwind_protect()`

* Tweak internals vignette advice

* NEWS bullet

* Tweaks from code review

Author: DavisVaughan

NEWS.md

@@ -1,5 +1,12 @@
 # cpp11 (development version)
 
+* Nested calls to `cpp11::unwind_protect()` are no longer supported or
+  encouraged. Previously, this was something that could be done for performance
+  improvements, but ultimately this feature has proven to cause more problems
+  than it is worth and is very hard to use safely. For more information, see the
+  new `vignette("FAQ")` section titled "Should I call `cpp11::unwind_protect()`
+  manually?" (#327).
+
 # cpp11 0.4.5
 
 * On 2023-07-20, cpp11 was temporarily rolled back to 0.4.3 manually by CRAN due

cpp11test/R/cpp11.R

@@ -223,3 +223,11 @@ rcpp_sum_dbl_accumulate_ <- function(x_sxp) {
 rcpp_grow_ <- function(n_sxp) {
   .Call(`_cpp11test_rcpp_grow_`, n_sxp)
 }
+
+test_destruction_inner <- function() {
+  invisible(.Call(`_cpp11test_test_destruction_inner`))
+}
+
+test_destruction_outer <- function() {
+  invisible(.Call(`_cpp11test_test_destruction_outer`))
+}

cpp11test/src/cpp11.cpp

@@ -422,10 +422,26 @@ extern "C" SEXP _cpp11test_rcpp_grow_(SEXP n_sxp) {
     return cpp11::as_sexp(rcpp_grow_(cpp11::as_cpp<cpp11::decay_t<SEXP>>(n_sxp)));
   END_CPP11
 }
+// test-protect-nested.cpp
+void test_destruction_inner();
+extern "C" SEXP _cpp11test_test_destruction_inner() {
+  BEGIN_CPP11
+    test_destruction_inner();
+    return R_NilValue;
+  END_CPP11
+}
+// test-protect-nested.cpp
+void test_destruction_outer();
+extern "C" SEXP _cpp11test_test_destruction_outer() {
+  BEGIN_CPP11
+    test_destruction_outer();
+    return R_NilValue;
+  END_CPP11
+}
 
 extern "C" {
 /* .Call calls */
-extern SEXP run_testthat_tests(SEXP);
+extern SEXP run_testthat_tests(void *);
 
 static const R_CallMethodDef CallEntries[] = {
     {"_cpp11test_col_sums",                 (DL_FUNC) &_cpp11test_col_sums,                 1},
@@ -483,6 +499,8 @@ static const R_CallMethodDef CallEntries[] = {
     {"_cpp11test_sum_int_for2_",            (DL_FUNC) &_cpp11test_sum_int_for2_,            1},
     {"_cpp11test_sum_int_for_",             (DL_FUNC) &_cpp11test_sum_int_for_,             1},
     {"_cpp11test_sum_int_foreach_",         (DL_FUNC) &_cpp11test_sum_int_foreach_,         1},
+    {"_cpp11test_test_destruction_inner",   (DL_FUNC) &_cpp11test_test_destruction_inner,   0},
+    {"_cpp11test_test_destruction_outer",   (DL_FUNC) &_cpp11test_test_destruction_outer,   0},
     {"_cpp11test_upper_bound",              (DL_FUNC) &_cpp11test_upper_bound,              2},
     {"run_testthat_tests",                  (DL_FUNC) &run_testthat_tests,                  1},
     {NULL, NULL, 0}

cpp11test/src/test-protect-nested.cpp

@@ -0,0 +1,81 @@
+#include "cpp11/function.hpp"
+#include "cpp11/protect.hpp"
+#include "testthat.h"
+
+#ifdef HAS_UNWIND_PROTECT
+
+/*
+ * See https://github.com/r-lib/cpp11/pull/327 for full details.
+ *
+ * - `cpp11::package("cpp11test")["test_destruction_outer"]` uses
+ *   `unwind_protect()` to call R level `test_destruction_outer()` but no entry
+ *   macros are set up. Instead we are going to catch exceptions that get here
+ *   with `expect_error_as()`.
+ *
+ *   - Call R level `test_destruction_outer()` to set up `BEGIN_CPP11` /
+ *     `END_CPP11` entry macros.
+ *
+ *   - C++ `test_destruction_outer()` goes through `unwind_protect()` to call
+ *     the R level `test_destruction_inner()`.
+ *
+ *     - R level `test_destruction_inner()` sets up its own `BEGIN_CPP11` /
+ *       `END_CPP11` entry macros.
+ *
+ *     - C++ `test_destruction_inner()` goes through `unwind_protect()` to call
+ *       `Rf_error()` (i.e., we are nested within `unwind_protect()`s!).
+ *
+ *     - `longjmp()` is caught from inner `unwind_protect()`, and an exception
+ *       is thrown which is caught by the inner entry macros, allowing us to run
+ *       the destructor of `x`, then we let R continue the unwind process.
+ *
+ *   - This `longjmp()`s again and is caught by the outer `unwind_protect()`, an
+ *     exception is thrown which is caught by the outer entry macros, and we let
+ *     R continue the unwind process one more time.
+ *
+ * - This `longjmp()` is caught by `cpp11::package()`'s `unwind_protect()`,
+ *   an exception is thrown, and that is caught by `expect_error_as()`.
+ */
+
+// Global variable to detect if the destructor has been run or not
+static bool destructed = false;
+
+class HasDestructor {
+ public:
+  ~HasDestructor();
+};
+
+HasDestructor::~HasDestructor() {
+  // Destructor has run!
+  destructed = true;
+}
+
+[[cpp11::register]] void test_destruction_inner() {
+  // Expect that `x`'s destructor gets to run on the way out
+  HasDestructor x{};
+  cpp11::stop("oh no!");
+}
+
+[[cpp11::register]] void test_destruction_outer() {
+  const auto test_destruction_inner =
+      cpp11::package("cpp11test")["test_destruction_inner"];
+  test_destruction_inner();
+}
+
+context("unwind_protect-nested-C++") {
+  test_that(
+      "nested `unwind_protect()` (with entry macros set up) will run destructors"
+      "(#327)") {
+    const auto fn = [&] {
+      const auto test_destruction_outer =
+          cpp11::package("cpp11test")["test_destruction_outer"];
+      test_destruction_outer();
+    };
+
+    expect_error_as(fn(), cpp11::unwind_exception);
+    expect_true(destructed);
+
+    destructed = false;
+  }
+}
+
+#endif

inst/include/cpp11/protect.hpp

@@ -54,36 +54,6 @@ inline void set_option(SEXP name, SEXP value) {
   SETCAR(opt, value);
 }
 
-inline Rboolean* setup_should_unwind_protect() {
-  SEXP should_unwind_protect_sym = Rf_install("cpp11_should_unwind_protect");
-  SEXP should_unwind_protect_sexp = Rf_GetOption1(should_unwind_protect_sym);
-
-  if (should_unwind_protect_sexp == R_NilValue) {
-    // Allocate and initialize once, then let R manage it.
-    // That makes this a shared global across all compilation units.
-    should_unwind_protect_sexp = PROTECT(Rf_allocVector(LGLSXP, 1));
-    SET_LOGICAL_ELT(should_unwind_protect_sexp, 0, TRUE);
-    detail::set_option(should_unwind_protect_sym, should_unwind_protect_sexp);
-    UNPROTECT(1);
-  }
-
-  return reinterpret_cast<Rboolean*>(LOGICAL(should_unwind_protect_sexp));
-}
-
-inline Rboolean* access_should_unwind_protect() {
-  // Setup is run once per compilation unit, but all compilation units
-  // share the same global option, so each compilation unit's static pointer
-  // will point to the same object.
-  static Rboolean* p_should_unwind_protect = setup_should_unwind_protect();
-  return p_should_unwind_protect;
-}
-
-inline Rboolean get_should_unwind_protect() { return *access_should_unwind_protect(); }
-
-inline void set_should_unwind_protect(Rboolean should_unwind_protect) {
-  *access_should_unwind_protect() = should_unwind_protect;
-}
-
 }  // namespace detail
 
 #ifdef HAS_UNWIND_PROTECT
@@ -94,12 +64,6 @@ inline void set_should_unwind_protect(Rboolean should_unwind_protect) {
 template <typename Fun, typename = typename std::enable_if<std::is_same<
                             decltype(std::declval<Fun&&>()()), SEXP>::value>::type>
 SEXP unwind_protect(Fun&& code) {
-  if (detail::get_should_unwind_protect() == FALSE) {
-    return std::forward<Fun>(code)();
-  }
-
-  detail::set_should_unwind_protect(FALSE);
-
   static SEXP token = [] {
     SEXP res = R_MakeUnwindCont();
     R_PreserveObject(res);
@@ -108,7 +72,6 @@ SEXP unwind_protect(Fun&& code) {
 
   std::jmp_buf jmpbuf;
   if (setjmp(jmpbuf)) {
-    detail::set_should_unwind_protect(TRUE);
     throw unwind_exception(token);
   }
 
@@ -133,8 +96,6 @@ SEXP unwind_protect(Fun&& code) {
   // unset it here before returning the value ourselves.
   SETCAR(token, R_NilValue);
 
-  detail::set_should_unwind_protect(TRUE);
-
   return res;
 }