Repository: extendr/extendr
Pull request #569 (2023-06-27 12:31:26)

ci: check msrv
Use cargo-msrv to guarantee the minimum Rust version supported.
https://crates.io/crates/cargo-msrv

multimeric: > A single run is sufficient to check MSRV, while other tests use the matrix to check multiple times.

True, all the tests in the main workflow use a matrix. This is enough reason to keep this separate.

Patch:
diff --git a/.github/workflows/msrv.yml b/.github/workflows/msrv.yml
new file mode 100644
index 000000000..8c8621f43
--- /dev/null
+++ b/.github/workflows/msrv.yml
@@ -0,0 +1,33 @@
+name: Check MSRV
+on:
+  push:
+    branches:
+      - master
+    paths:
+      - "**.rs"
+      - "**/Cargo.toml"
+  pull_request:
+    branches:
+      - master
+    paths:
+      - "**.rs"
+      - "**/Cargo.toml"
+  workflow_dispatch:
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  test-min-rust-version:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: baptiste0928/cargo-install@v2
+        with:
+          crate: cargo-msrv
+      - name: Verify minimum rust version
+        run: cargo-msrv --path extendr-api/ verify
diff --git a/.gitignore b/.gitignore
index 32be9e25d..02a68352a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,3 +34,4 @@ extendr.Rproj
 tests/extendrtests.Rcheck/
 tests/extendrtests_0.1.0.tar.gz
 **/.cargo/*
+.vscode/settings.json
diff --git a/CHANGELOG.md b/CHANGELOG.md
index fbb3e2a2e..7719b4a28 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@
     [[#501]](https://github.com/extendr/extendr/pull/501)
 - Raw literals from Rust can be used for function and argument names. e.g. `fn r#type()` in Rust is converted to `type()` in R.
     [[#531]](https://github.com/extendr/extendr/pull/531)
+- Fix memory leaks on errors and panics
+    [[#555]](https://github.com/extendr/extendr/pull/555)
 
 ## 0.4.0
 
diff --git a/extendr-api/Cargo.toml b/extendr-api/Cargo.toml
index 6e3d1edd7..2c1b805ba 100644
--- a/extendr-api/Cargo.toml
+++ b/extendr-api/Cargo.toml
@@ -14,6 +14,7 @@ edition = "2021"
 description = "Safe and user friendly bindings to the R programming language."
 license = "MIT"
 repository = "https://github.com/extendr/extendr"
+rust-version = "1.60"
 
 [dependencies]
 libR-sys = "0.5.0"
diff --git a/extendr-api/src/lib.rs b/extendr-api/src/lib.rs
index 3292429b1..a0d14909f 100644
--- a/extendr-api/src/lib.rs
+++ b/extendr-api/src/lib.rs
@@ -547,11 +547,12 @@ pub fn sxp_to_rtype(sxptype: i32) -> Rtype {
     }
 }
 
+const PRINTF_NO_FMT_CSTRING: &'static [i8] = &[37, 115, 0]; // same as "%s\0"
 #[doc(hidden)]
 pub fn print_r_output<T: Into<Vec<u8>>>(s: T) {
     let cs = CString::new(s).expect("NulError");
     unsafe {
-        Rprintf(cs.as_ptr());
+        Rprintf(PRINTF_NO_FMT_CSTRING.as_ptr(), cs.as_ptr());
     }
 }
 
@@ -559,7 +560,7 @@ pub fn print_r_output<T: Into<Vec<u8>>>(s: T) {
 pub fn print_r_error<T: Into<Vec<u8>>>(s: T) {
     let cs = CString::new(s).expect("NulError");
     unsafe {
-        REprintf(cs.as_ptr());
+        REprintf(PRINTF_NO_FMT_CSTRING.as_ptr(), cs.as_ptr());
     }
 }
 
@@ -873,11 +874,11 @@ mod tests {
         test! {
             let txt_con = R!(r#"textConnection("test_con", open = "w")"#).unwrap();
             call!("sink", &txt_con).unwrap();
-            rprintln!("Hello world");
+            rprintln!("Hello world %%!"); //%% checks printf formatting is off, yields one % if on
             call!("sink").unwrap();
             call!("close", &txt_con).unwrap();
             let result = R!("test_con").unwrap();
-            assert_eq!(result, r!("Hello world"));
+            assert_eq!(result, r!("Hello world %%!"));
         }
     }
 
diff --git a/extendr-macros/src/wrappers.rs b/extendr-macros/src/wrappers.rs
index 644cb2cf6..12fa4e1f7 100644
--- a/extendr-macros/src/wrappers.rs
+++ b/extendr-macros/src/wrappers.rs
@@ -44,8 +44,6 @@ pub fn make_function_wrappers(
     let doc_string = get_doc_string(attrs);
     let return_type_string = get_return_type(sig);
 
-    let panic_str = format!("{} panicked.\0", r_name_str);
-
     let inputs = &mut sig.inputs;
     let has_self = matches!(inputs.iter().next(), Some(FnArg::Receiver(_)));
 
@@ -102,17 +100,48 @@ pub fn make_function_wrappers(
     //     }
     // }
     // ```
+    //
     wrappers.push(parse_quote!(
         #[no_mangle]
         #[allow(non_snake_case, clippy::not_unsafe_ptr_arg_deref)]
         pub extern "C" fn #wrap_name(#formal_args) -> extendr_api::SEXP {
-            unsafe {
-                use extendr_api::robj::*;
+            use extendr_api::robj::*;
+            let wrap_result_state: std::result::Result<
+                std::result::Result<Robj, extendr_api::Error>,
+                Box<dyn std::any::Any + Send>
+            > = unsafe {
                 #( #convert_args )*
-                extendr_api::handle_panic(#panic_str, ||
-                    extendr_api::Robj::from(#call_name(#actual_args)).get()
-                )
+                std::panic::catch_unwind(||-> std::result::Result<Robj, extendr_api::Error> {
+                    Ok(extendr_api::Robj::from(#call_name(#actual_args)))
+                })
+            };
+            // any obj created in above unsafe scope, which are not moved into wrap_result_state are now dropped
+
+
+            match wrap_result_state {
+                Ok(Ok(zz)) => {
+                    return unsafe { zz.get() };
+                }
+                // any conversion error bubbled from #actual_args conversions of incomming args from R.
+                Ok(Err(conversion_err)) => {
+                    let err_string = conversion_err.to_string();
+                    drop(conversion_err); // try_from=true errors contain Robj, this must be dropped to not leak
+                    extendr_api::throw_r_error(&err_string);
+                }
+                // any panic (induced by user func code or if user func yields a Result-Err as return value)
+                Err(unwind_err) => {
+                    drop(unwind_err); //did not notice any difference if dropped or not.
+                    // It should be possible to downcast the unwind_err Any type to the error
+                    // included in panic. The advantage would be the panic cause could be included
+                    // in the R terminal error message and not only via std-err.
+                    // but it should be handled in a separate function and not in-lined here.
+                    let err_string = format!("user function panicked: {}\0",#r_name_str);
+                    // cannot use throw_r_error here for some reason.
+                    // handle_panic() exports err string differently than throw_r_error.
+                    extendr_api::handle_panic(err_string.as_str(), || panic!());
+                }
             }
+            unreachable!("internal extendr error, this should never happen.")
         }
     ));
 
@@ -302,12 +331,11 @@ fn translate_actual(opts: &ExtendrOptions, input: &FnArg) -> Option<Expr> {
             if let syn::Pat::Ident(ref ident) = pat {
                 let varname = format_ident!("_{}_robj", ident.ident);
                 if opts.use_try_from {
-                    Some(parse_quote! { extendr_api::unwrap_or_throw_error(
-                        #varname.try_into()
-                        .map_err(|e| extendr_api::Error::from(e)))
+                    Some(parse_quote! {
+                        #varname.try_into()?
                     })
                 } else {
-                    Some(parse_quote! { extendr_api::unwrap_or_throw(<#ty>::from_robj(&#varname)) })
+                    Some(parse_quote! { <#ty>::from_robj(&#varname)? })
                 }
             } else {
                 None


Test patch:
diff --git a/tests/extendrtests/DESCRIPTION b/tests/extendrtests/DESCRIPTION
index 513a0ef6b..30008f108 100644
--- a/tests/extendrtests/DESCRIPTION
+++ b/tests/extendrtests/DESCRIPTION
@@ -23,5 +23,6 @@ Suggests:
     rmarkdown,
     stringi,
     testthat,
-    vctrs
-Config/rextendr/version: 0.2.0.9000
+    vctrs,
+    lobstr
+Config/rextendr/version: 0.3.0
diff --git a/tests/extendrtests/R/extendr-wrappers.R b/tests/extendrtests/R/extendr-wrappers.R
index c814bde21..cad294d2c 100644
--- a/tests/extendrtests/R/extendr-wrappers.R
+++ b/tests/extendrtests/R/extendr-wrappers.R
@@ -106,6 +106,22 @@ true <- function() .Call(wrap__true)
 #' @export
 false <- function(type) .Call(wrap__false, type)
 
+leak_implicit_strings <- function(x) .Call(wrap__leak_implicit_strings, x)
+
+leak_implicit_doubles <- function(x) .Call(wrap__leak_implicit_doubles, x)
+
+leak_arg2_try_implicit_strings <- function(`_y`, x) .Call(wrap__leak_arg2_try_implicit_strings, `_y`, x)
+
+leak_arg2_try_implicit_doubles <- function(`_y`, x) .Call(wrap__leak_arg2_try_implicit_doubles, `_y`, x)
+
+leak_unwrap_strings <- function(x) .Call(wrap__leak_unwrap_strings, x)
+
+leak_unwrap_doubles <- function(x) .Call(wrap__leak_unwrap_doubles, x)
+
+leak_positive_control <- function(x) invisible(.Call(wrap__leak_positive_control, x))
+
+leak_negative_control <- function(x) invisible(.Call(wrap__leak_negative_control, x))
+
 #' Class for testing (exported)
 #' @examples
 #' x <- MyClass$new()
diff --git a/tests/extendrtests/README.md b/tests/extendrtests/README.md
index 5f41d077c..d9637f4f7 100644
--- a/tests/extendrtests/README.md
+++ b/tests/extendrtests/README.md
@@ -23,3 +23,4 @@ rcmdcheck::rcmdcheck(check_dir = "../../../")
 This is necessary so the relative paths in Cargo.toml points to the correct location.
 
 Clicking "Check" in RStudio will not work because it runs `devtools::check()` (that resolves to `rcmdcheck::rcmdcheck()`) with no arguments and the checks are then run in a temporary directory that doesn't have the same relative path.
+
diff --git a/tests/extendrtests/src/rust/src/lib.rs b/tests/extendrtests/src/rust/src/lib.rs
index a79f1a0c7..07b3ae413 100644
--- a/tests/extendrtests/src/rust/src/lib.rs
+++ b/tests/extendrtests/src/rust/src/lib.rs
@@ -10,6 +10,8 @@ mod optional_either;
 
 mod raw_identifiers;
 
+mod memory_leaks;
+
 // Return string `"Hello world!"` to R.
 #[extendr]
 fn hello_world() -> &'static str {
@@ -360,4 +362,5 @@ extendr_module! {
     use optional_ndarray;
     use optional_either;
     use raw_identifiers;
+    use memory_leaks;
 }
diff --git a/tests/extendrtests/src/rust/src/memory_leaks.rs b/tests/extendrtests/src/rust/src/memory_leaks.rs
new file mode 100644
index 000000000..986b2c8e7
--- /dev/null
+++ b/tests/extendrtests/src/rust/src/memory_leaks.rs
@@ -0,0 +1,59 @@
+use extendr_api::prelude::*;
+
+#[extendr]
+fn leak_implicit_strings(x: Strings) -> String {
+    x.len().to_string()
+}
+
+#[extendr]
+fn leak_implicit_doubles(x: Doubles) -> String {
+    x.len().to_string()
+}
+
+#[extendr(use_try_from = true)]
+fn leak_arg2_try_implicit_strings(_y: Doubles, x: Strings) -> String {
+    x.len().to_string()
+}
+
+#[extendr(use_try_from = true)]
+fn leak_arg2_try_implicit_doubles(_y: Doubles, x: Doubles) -> String {
+    x.len().to_string()
+}
+
+#[extendr]
+fn leak_unwrap_strings(x: Robj) -> String {
+    let x = x.as_string_vector().ok_or("ERROR").unwrap();
+    x.len().to_string()
+}
+
+#[extendr]
+fn leak_unwrap_doubles(x: Robj) -> String {
+    x.as_real_vector().ok_or("ERROR").unwrap().len().to_string()
+}
+
+#[extendr]
+fn leak_positive_control(x: Robj) {
+    std::mem::forget(x);
+}
+
+#[extendr]
+fn leak_negative_control(x: Robj) {
+    drop(x)
+}
+
+// Macro to generate exports.
+// This ensures exported functions are registered with R.
+// See corresponding C code in `entrypoint.c`.
+extendr_module! {
+    mod memory_leaks;
+
+    fn leak_implicit_strings;
+    fn leak_implicit_doubles;
+    fn leak_arg2_try_implicit_strings;
+    fn leak_arg2_try_implicit_doubles;
+    fn leak_unwrap_strings;
+    fn leak_unwrap_doubles;
+    fn leak_positive_control;
+    fn leak_negative_control;
+
+}
diff --git a/tests/extendrtests/tests/testthat/test-memory_leaks.R b/tests/extendrtests/tests/testthat/test-memory_leaks.R
new file mode 100644
index 000000000..dd2801d39
--- /dev/null
+++ b/tests/extendrtests/tests/testthat/test-memory_leaks.R
@@ -0,0 +1,133 @@
+
+
+
+test_that("extendr conversions/panics does not leak", {
+
+  ## get list of rust_functions and value generator functions to test.
+  e_pkg = environment(leak_implicit_doubles)
+  fnames = c(
+
+    "leak_implicit_doubles",# single burn-in function
+
+    "leak_implicit_doubles",
+    "leak_implicit_strings",
+
+    "leak_arg2_try_implicit_doubles",
+    "leak_arg2_try_implicit_strings"
+  )
+  rust_f_list = mget(fnames,envir = e_pkg)
+  names(rust_f_list)[1] = "burn_in_test" #rename as burn-in
+
+  value_f_list = list(
+    big_chrvec = \() replicate(1E1,paste(sample(letters,1E1, replace = TRUE),collapse = "")),
+    big_altvec = \() 1:(1E2),
+    big_dblvec = \() (1:(1E2)) - 1.1
+  )
+
+  ## global memory, to minimize allocations while measuring
+  glb_i = 0
+  glb_mem_before      = NA_real_
+  glb_mem_before_10   = NA_real_
+  glb_mem_after       = NA_real_
+  glb_mem_after_10    = NA_real_
+  glb_mem_after_gc    = NA_real_
+  glb_mem_after_gc_10 = NA_real_
+  glb_is_error = NA
+
+
+  #a function to measure
+  score_leak <- function(f_rust, f_value, verbose = FALSE, n_repeats = 2){
+
+    glb_mem_before <<- lobstr::mem_used()
+
+    #run rust function possibly with the wrong arg types, catch error
+    out = (\() tryCatch({
+      #two arg case
+      if(length(formals(f_rust))>1 ) {
+          f_rust(rnorm(1E2),f_value())
+      #one arg case
+      } else {
+          f_rust(f_value())
+        }
+      }, error = \(err) "ERROR")
+    )()
+
+    #measure mem usage and garbage collect and measure again
+    glb_mem_after <<- lobstr::mem_used()
+    glb_is_error <<- isTRUE(out == "ERROR")
+    rm(out)
+    gc(verbose = FALSE)
+    glb_mem_after_gc <<- lobstr::mem_used()
+
+
+    # now run again n_repeats
+    glb_mem_before_10 <<- lobstr::mem_used()
+    for (i in 1:n_repeats) {
+      out = (\() tryCatch({
+        if(length(formals(f_rust))>1 ) {
+          f_rust(rnorm(1E2),f_value())
+        } else {
+          f_rust(f_value())
+        }
+      }, error = \(err) "ERROR"))()
+    }
+    glb_mem_after_10 <<- lobstr::mem_used()
+
+    # drop garbage collect measure
+    rm(out,i)
+    gc(verbose = FALSE)
+    glb_mem_after_gc_10 <<- lobstr::mem_used()
+
+
+    # collect results
+    list(
+      total_mem_before = glb_mem_before,
+      is_error = glb_is_error,
+      leak_size_1  = glb_mem_after_gc -glb_mem_before,
+      leak_size_repeat = (glb_mem_after_gc_10 - glb_mem_before_10) / n_repeats
+    )
+  }
+
+
+  for(rust_fun_name in  names(rust_f_list)) {
+    for(value_fun_name in names(value_f_list)) {
+
+      mem_result = score_leak(
+        rust_f_list[[rust_fun_name]], value_f_list[[value_fun_name]]
+      )
+      #print(mem_result$total_mem_before)
+      if(rust_fun_name != "burn_in_test") {
+
+        expect_identical(
+          list(
+            rust_f = rust_fun_name,
+            input_val = value_fun_name,
+            leak = as.double(mem_result$leak_size_repeat)
+          ),
+          list(
+            rust_f = rust_fun_name,
+            input_val = value_fun_name,
+            leak = 0
+          )
+        )
+      }
+
+
+    }
+  }
+
+ #This test verifies score_leak() can detect a ~40kb leak
+  #isch 5000*8~40000~40kb is expected leaked
+  expect_true(
+    as.numeric(score_leak(leak_positive_control,\()rnorm(5000))$leak_size_repeat) > 10000
+  )
+
+  # This test verifies score_leak will always find less than 256 bytes for a negative control
+  # Without burn_in there is an increment of 128 bytes, which is not
+  # accounted for. However at about ~10 more runs this 128bytes per run are released at once.
+  expect_true(
+    as.numeric(score_leak(leak_negative_control,\()rnorm(5000))$leak_size_repeat) <= 256
+  )
+  
+})
+