From 950cb7a30f27b885b209938c1ff7df9561c7a8f9 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 2 Aug 2023 16:13:56 -0400 Subject: [PATCH 1/4] Depend on R >= 3.5.0 for `R_UnwindProtect()` At this point in r-lib and the tidyverse we typically require R >= 3.6.0, so it is reasonable to finally require this and remove our suboptimal fallback paths. --- DESCRIPTION | 2 ++ 1 file changed, 2 insertions(+) diff --git a/DESCRIPTION b/DESCRIPTION index d5cbbcc4..e0ead8d9 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -17,6 +17,8 @@ Description: Provides a header only, C++11 interface to R's C License: MIT + file LICENSE URL: https://cpp11.r-lib.org, https://github.com/r-lib/cpp11 BugReports: https://github.com/r-lib/cpp11/issues +Depends: + R (>= 3.5.0) Suggests: bench, brio, From a1a85fdafc955e2d4dbac50bac00429f45f09786 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 2 Aug 2023 16:24:05 -0400 Subject: [PATCH 2/4] Remove `release_all()` and `CPP11_USE_PRESERVE_OBJECT` Which no longer make sense if we have 1 preserve list per compilation unit and require R >=3.5.0. It does not seem to be used by anyone. --- inst/include/cpp11/protect.hpp | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/inst/include/cpp11/protect.hpp b/inst/include/cpp11/protect.hpp index 17da8a59..6f909601 100644 --- a/inst/include/cpp11/protect.hpp +++ b/inst/include/cpp11/protect.hpp @@ -253,13 +253,6 @@ static struct { return R_NilValue; } -#ifdef CPP11_USE_PRESERVE_OBJECT - PROTECT(obj); - R_PreserveObject(obj); - UNPROTECT(1); - return obj; -#endif - PROTECT(obj); static SEXP list_ = get_preserve_list(); @@ -290,29 +283,11 @@ static struct { REprintf("---\n"); } - // This is currently unused, but client packages could use it to free leaked resources - // in older R versions if needed - void release_all() { -#if !defined(CPP11_USE_PRESERVE_OBJECT) - static SEXP list_ = get_preserve_list(); - SEXP first = CDR(list_); - if (first != R_NilValue) { - SETCAR(first, R_NilValue); - SETCDR(list_, R_NilValue); - } -#endif - } - void release(SEXP cell) { if (cell == R_NilValue) { return; } -#ifdef CPP11_USE_PRESERVE_OBJECT - R_ReleaseObject(cell); - return; -#endif - // Get a reference to the cells before and after the token. SEXP lhs = CAR(cell); SEXP rhs = CDR(cell); From 31b7f9cc94fa7a2c081851d22e823891b8b16979 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 2 Aug 2023 16:24:27 -0400 Subject: [PATCH 3/4] Update unwind-protect section of internals vignette --- vignettes/internals.Rmd | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/vignettes/internals.Rmd b/vignettes/internals.Rmd index 0a8d2ef9..2eca4989 100644 --- a/vignettes/internals.Rmd +++ b/vignettes/internals.Rmd @@ -130,27 +130,24 @@ These functions are defined in [protect.hpp](https://github.com/r-lib/cpp11/blob ### Unwind Protect -In R 3.5+ cpp11 uses `R_UnwindProtect` to protect (most) calls to the R API that could fail. +cpp11 uses `R_UnwindProtect()` to protect (most) calls to the R API that could fail. These are usually those that allocate memory, though in truth most R API functions could error along some paths. -If an error happends under `R_UnwindProtect` cpp11 will throw a C++ exception. -This exception is caught by the try catch block defined in the `BEGIN_CPP11` macro in [cpp11/declarations.hpp](https://github.com/r-lib/cpp11/blob/main/inst/include/cpp11/declarations.hpp). +If an error happens under `R_UnwindProtect()`, cpp11 will throw a C++ exception. +This exception is caught by the try/catch block defined in the `BEGIN_CPP11` macro in [cpp11/declarations.hpp](https://github.com/r-lib/cpp11/blob/main/inst/include/cpp11/declarations.hpp). The exception will cause any C++ destructors to run, freeing any resources held by C++ objects. -After the try catch block exits the R error unwinding is then continued by `R_ContinueUnwind()` and a normal R error results. +After the try/catch block exits, the R error unwinding is then continued by `R_ContinueUnwind()` and a normal R error results. -In R versions prior to 3.5 `R_UnwindProtect()` is not available. -Unfortunately the options to emulate it are not ideal. +We require R >=3.5 to use cpp11, but when it was created we wanted to support back to R 3.3, but `R_ContinueUnwind()` wasn't available until R 3.5. +Below are a few other options we considered to support older R versions: 1. Using `R_TopLevelExec()` works to avoid the C long jump, but because the code is always run in a top level context any errors or messages thrown cannot be caught by `tryCatch()` or similar techniques. 2. Using `R_TryCatch()` is not available prior to R 3.4, and also has a serious bug in R 3.4 (fixed in R 3.5). 3. Calling the R level `tryCatch()` function which contains an expression that runs a C function which then runs the C++ code would be an option, but implementing this is convoluted and it would impact performance, perhaps severely. -4. Have `cpp11::unwind_protect()` be a no-op for these versions. This means any resources held by C++ objects would leak, including cpp11::r_vector / cpp11::sexp objects. +4. Have `cpp11::unwind_protect()` be a no-op for these versions. This means any resources held by C++ objects would leak, including `cpp11::r_vector` / `cpp11::sexp` objects. -None of these options is perfect, here are some pros and cons for each. +None of these options were perfect, here are some pros and cons for each. 1. Causes behavior changes and test failures, so it was ruled out. -2. Was also ruled out since we want to support back to R 3.3. +2. Was also ruled out since we wanted to support back to R 3.3. 3. Was ruled out partially because the implementation would be somewhat tricky and more because performance would suffer greatly. -4. is what we now do in cpp11. It leaks protected objects when there are R API errors. - -If packages are concerned about the leaked memory they can call `cpp11::preserved.release_all()` as needed to release the current protections for all objects managed by cpp11. -This is not done automatically because in some cases the protections should persist beyond the `.Call()` boundry, e.g. in vroom altrep objects for example. +4. Is what we ended up doing before requiring R 3.5. It leaked protected objects when there were R API errors. From 398ac946931164d73f7e8d95b372e573cdae66c4 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Wed, 2 Aug 2023 16:30:31 -0400 Subject: [PATCH 4/4] NEWS bullet --- NEWS.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/NEWS.md b/NEWS.md index 0aa0aa0a..919ed935 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,16 @@ # cpp11 (development version) +* R >=3.5.0 is now required to use cpp11. This is in line with (and even goes + beyond) the tidyverse standard of supporting the previous 5 minor releases of + R. It also ensures that `R_UnwindProtect()` is available to avoid C++ memory + leaks (#332). + +* `cpp11::preserved.release_all()` has been removed. This was intended to + support expert developers on R <3.5.0 when cpp11 used a global protection + list. Since cpp11 no longer uses a global protection list and requires R + >=3.5.0, it is no longer needed. As far as we can tell, no package was + actively using this (#332). + * 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