From cb2ab165c3dc313145718f6206653c0d7edaf3d4 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 2 Aug 2023 15:54:04 -0400 Subject: [PATCH 1/3] Use 1 preserve list per compilation unit --- inst/include/cpp11/protect.hpp | 87 ++++++---------------------------- vignettes/internals.Rmd | 3 ++ 2 files changed, 17 insertions(+), 73 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 7ff4a47f..a7651f73 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -31,31 +31,6 @@ class unwind_exception : public std::exception { unwind_exception(SEXP token_) : token(token_) {} }; -namespace detail { -// We deliberately avoid using safe[] in the below code, as this code runs -// when the shared library is loaded and will not be wrapped by -// `CPP11_UNWIND`, so if an error occurs we will not catch the C++ exception -// that safe emits. -inline void set_option(SEXP name, SEXP value) { - static SEXP opt = SYMVALUE(Rf_install(".Options")); - SEXP t = opt; - while (CDR(t) != R_NilValue) { - if (TAG(CDR(t)) == name) { - opt = CDR(t); - SET_TAG(opt, name); - SETCAR(opt, value); - return; - } - t = CDR(t); - } - SETCDR(t, Rf_allocList(1)); - opt = CDR(t); - SET_TAG(opt, name); - SETCAR(opt, value); -} - -} // namespace detail - #ifdef HAS_UNWIND_PROTECT /// Unwind Protection from C longjmp's, like those used in R error handling @@ -350,58 +325,24 @@ static struct { } private: - // The preserved list singleton is stored in a XPtr within an R global option. - // - // It is not constructed as a static variable directly since many - // translation units may be compiled, resulting in unrelated instances of each - // static variable. - // - // We cannot store it in the cpp11 namespace, as cpp11 likely will not be loaded by - // packages. - // We cannot store it in R's global environment, as that is against CRAN - // policies. - // We instead store it as an XPtr in the global options, which avoids issues - // both copying and serializing. - static SEXP get_preserve_xptr_addr() { - static SEXP preserve_xptr_sym = Rf_install("cpp11_preserve_xptr"); - SEXP preserve_xptr = Rf_GetOption1(preserve_xptr_sym); - - if (TYPEOF(preserve_xptr) != EXTPTRSXP) { - return R_NilValue; - } - auto addr = R_ExternalPtrAddr(preserve_xptr); - if (addr == nullptr) { - return R_NilValue; - } - return static_cast(addr); - } + // Each compilation unit purposefully gets its own preserve list. + // This avoids issues with sharing preserve list state across compilation units + // and across packages, which has historically caused many issues (#330). + static SEXP get_preserve_list() { + static SEXP out = R_NilValue; - static void set_preserve_xptr(SEXP value) { - static SEXP preserve_xptr_sym = Rf_install("cpp11_preserve_xptr"); + if (out == R_NilValue) { + // Initialize the list exactly once per compilation unit, + // and let R manage its memory + out = new_preserve_list(); + R_PreserveObject(out); + } - SEXP xptr = PROTECT(R_MakeExternalPtr(value, R_NilValue, R_NilValue)); - detail::set_option(preserve_xptr_sym, xptr); - UNPROTECT(1); + return out; } - static SEXP get_preserve_list() { - static SEXP preserve_list = R_NilValue; - if (TYPEOF(preserve_list) != LISTSXP) { - preserve_list = get_preserve_xptr_addr(); - if (TYPEOF(preserve_list) != LISTSXP) { - preserve_list = Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue)); - R_PreserveObject(preserve_list); - set_preserve_xptr(preserve_list); - } - - // NOTE: Because older versions of cpp11 (<= 0.4.2) initialized the - // precious_list with a single cell, we might need to detect and update - // an existing empty precious list so that we have a second cell following. - if (CDR(preserve_list) == R_NilValue) - SETCDR(preserve_list, Rf_cons(R_NilValue, R_NilValue)); - } - - return preserve_list; + static SEXP new_preserve_list() { + return Rf_cons(R_NilValue, Rf_cons(R_NilValue, R_NilValue)); } } preserved; diff --git a/vignettes/internals.Rmd b/vignettes/internals.Rmd index 73d7de05..0a8d2ef9 100644 --- a/vignettes/internals.Rmd +++ b/vignettes/internals.Rmd @@ -123,6 +123,9 @@ Calling `preserved.release()` on this returned token will release the protection This scheme scales in O(1) time to release or insert an object vs O(N) or worse time with `R_PreserveObject()` / `R_ReleaseObject()`. +Each compilation unit has its own unique protection list, which avoids the need to manage a "global" protection list shared across compilation units within the same package and across packages. +A previous version of cpp11 used a global protection list, but this caused [multiple issues](https://github.com/r-lib/cpp11/issues/330). + These functions are defined in [protect.hpp](https://github.com/r-lib/cpp11/blob/main/inst/include/cpp11/protect.hpp) ### Unwind Protect From 3659e5570b451fce13d0a30a9234f027aba7eee4 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 2 Aug 2023 15:54:12 -0400 Subject: [PATCH 2/3] NEWS bullet --- NEWS.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.md b/NEWS.md index e657d5be..0aa0aa0a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,11 @@ # cpp11 (development version) +* cpp11 now creates one protection list per compilation unit, rather than one global + protection list shared across compilation units and across packages. This greatly + reduces the complexity of managing the protection list state and should make it easier + to make changes to the protection list structure in the future without breaking packages + compiled with older versions of cpp11 (#330). + * 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 From d95c0e28bef25c4df5104c9dd35e357da517da57 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 3 Aug 2023 08:32:16 -0400 Subject: [PATCH 3/3] Use dynamic initialization --- inst/include/cpp11/protect.hpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index a7651f73..17da8a59 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -329,15 +329,15 @@ static struct { // This avoids issues with sharing preserve list state across compilation units // and across packages, which has historically caused many issues (#330). static SEXP get_preserve_list() { - static SEXP out = R_NilValue; - - if (out == R_NilValue) { - // Initialize the list exactly once per compilation unit, - // and let R manage its memory - out = new_preserve_list(); - R_PreserveObject(out); - } + static SEXP out = init_preserve_list(); + return out; + } + static SEXP init_preserve_list() { + // Initialize the list exactly once per compilation unit, + // and let R manage its memory + SEXP out = new_preserve_list(); + R_PreserveObject(out); return out; }