From 9f9d224a265f04b3667b1db0c0c58e6a0a24aa31 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 10 Jan 2025 11:56:12 -0500 Subject: [PATCH 1/3] Move `R_BANNER` into `RMain` --- crates/ark/src/interface.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 1328e2f05..c3b75dfcc 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -162,9 +162,6 @@ thread_local! { pub static R_MAIN: RefCell> = panic!("Must access `R_MAIN` from the R thread"); } -/// Banner output accumulated during startup -static mut R_BANNER: String = String::new(); - pub struct RMain { kernel_init_tx: Bus, @@ -229,6 +226,9 @@ pub struct RMain { pub positron_ns: Option, pending_lines: Vec, + + /// Banner output accumulated during startup + r_banner: String, } /// Represents the currently active execution request from the frontend. It @@ -513,7 +513,7 @@ impl RMain { let kernel_info = KernelInfo { version: version.clone(), - banner: R_BANNER.clone(), + banner: self.r_banner.clone(), input_prompt: Some(input_prompt), continuation_prompt: Some(continuation_prompt), }; @@ -563,6 +563,7 @@ impl RMain { session_mode, positron_ns: None, pending_lines: Vec::new(), + r_banner: String::new(), } } @@ -1590,14 +1591,14 @@ impl RMain { Err(err) => panic!("Failed to read from R buffer: {err:?}"), }; + let r_main = RMain::get_mut(); + if !RMain::is_initialized() { // During init, consider all output to be part of the startup banner - unsafe { R_BANNER.push_str(&content) }; + r_main.r_banner.push_str(&content); return; } - let r_main = RMain::get_mut(); - // To capture the current `debug: ` output, for use in the debugger's // match based fallback r_main.dap.handle_stdout(&content); From a88557010f9f47513fd57b71c7bd3dceb3595a50 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 10 Jan 2025 12:08:39 -0500 Subject: [PATCH 2/3] Move `ERROR_BUF` to `RMain` This works only because of the `UnsafeCell` around `RMain` --- crates/ark/src/interface.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index c3b75dfcc..b3b1dca13 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -229,6 +229,10 @@ pub struct RMain { /// Banner output accumulated during startup r_banner: String, + + /// Raw error buffer provided to `Rf_error()` when throwing `r_read_console()` errors. + /// Stored in `RMain` to avoid memory leakage when `Rf_error()` jumps. + r_error_buffer: Option, } /// Represents the currently active execution request from the frontend. It @@ -564,6 +568,7 @@ impl RMain { positron_ns: None, pending_lines: Vec::new(), r_banner: String::new(), + r_error_buffer: None, } } @@ -1951,17 +1956,12 @@ pub extern "C" fn r_read_console( return 0; }, ConsoleResult::Error(err) => { - // Save error message to a global buffer to avoid leaking memory + // Save error message to `RMain`'s buffer to avoid leaking memory // when `Rf_error()` jumps. Some gymnastics are required to deal // with the possibility of `CString` conversion failure since the // error message comes from the frontend and might be corrupted. - static mut ERROR_BUF: Option = None; - - unsafe { - ERROR_BUF = Some(new_cstring(format!("\n{err}"))); - } - - unsafe { Rf_error(ERROR_BUF.as_ref().unwrap().as_ptr()) }; + main.r_error_buffer = Some(new_cstring(format!("\n{err}"))); + unsafe { Rf_error(main.r_error_buffer.as_ref().unwrap().as_ptr()) }; }, }; } From ffa79a0e9ac8ff2107e18138078038bed358c6a4 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 10 Jan 2025 12:11:05 -0500 Subject: [PATCH 3/3] Remove unused `r_version()` Seems easier than resolving the `R_VERSION` issues, we can always add it back if we need it, but we don't really need it with our current libr setup. --- crates/harp/src/lib.rs | 1 - crates/harp/src/r_version.rs | 60 ------------------------------------ 2 files changed, 61 deletions(-) delete mode 100644 crates/harp/src/r_version.rs diff --git a/crates/harp/src/lib.rs b/crates/harp/src/lib.rs index 18f12db46..00099ab0d 100644 --- a/crates/harp/src/lib.rs +++ b/crates/harp/src/lib.rs @@ -26,7 +26,6 @@ pub mod parse; pub mod parser; pub mod polled_events; pub mod protect; -pub mod r_version; pub mod raii; pub mod routines; pub mod session; diff --git a/crates/harp/src/r_version.rs b/crates/harp/src/r_version.rs deleted file mode 100644 index acdc126d5..000000000 --- a/crates/harp/src/r_version.rs +++ /dev/null @@ -1,60 +0,0 @@ -// -// r_version.rs -// -// Copyright (C) 2023 Posit Software, PBC. All rights reserved. -// -// - -use std::mem::MaybeUninit; -use std::sync::Once; - -use semver::Version; -use stdext::unwrap; - -use crate::exec::RFunction; -use crate::exec::RFunctionExt; - -/// Determine the running R version -/// -/// Determining the running R version from within ark -/// is complicated by the fact that there is no C function -/// in the R API that returns the version dynamically. To determine the -/// running R version, we call out to R once and store the result. -pub unsafe fn r_version() -> &'static Version { - static INIT_R_VERSION: Once = Once::new(); - static mut R_VERSION: MaybeUninit = MaybeUninit::uninit(); - - INIT_R_VERSION.call_once(|| { - R_VERSION.write(unsafe { r_version_impl() }); - }); - - unsafe { R_VERSION.assume_init_ref() } -} - -unsafe fn r_version_impl() -> Version { - let result = unwrap!(RFunction::new("base", "getRversion").call(), Err(error) => { - log::error!("Failed in `getRversion()` due to: {error:?}."); - return r_version_fallback(); - }); - - let result = unwrap!(RFunction::new("base", "as.character").add(result).call(), Err(error) => { - log::error!("Failed in `as.character()` due to: {error:?}."); - return r_version_fallback(); - }); - - let result = unwrap!(result.to::(), Err(error) => { - log::error!("Failed to convert version to string due to: {error:?}."); - return r_version_fallback(); - }); - - let version = unwrap!(semver::Version::parse(&result), Err(error) => { - log::error!("Failed to parse version due to: {error:?}."); - return r_version_fallback(); - }); - - version -} - -unsafe fn r_version_fallback() -> Version { - semver::Version::new(0, 0, 0) -}