POC for r_lock! enforcement on the R API
#26
Conversation
| if let Err(err) = r_assert_type(env.sexp, &[ENVSXP]) { | ||
| warn!( | ||
| "Environment: Attempt to monitor or list non-environment object {:?} ({:?})", | ||
| env, err | ||
| ); | ||
| r_lock! { | ||
| if let Err(err) = r_assert_type(env.sexp, &[ENVSXP]) { | ||
| warn!( | ||
| "Environment: Attempt to monitor or list non-environment object {:?} ({:?})", | ||
| env, err | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing r_lock! when starting the environment pane, one could argue whether or not TYPEOF() should require the lock
| // Get necessary prompts | ||
| let (continue_prompt, default_prompt) = | ||
| r_lock! { (r_get_option::<String>("continue"), r_get_option::<String>("prompt")) }; | ||
|
|
||
| // The request is incomplete if we see the continue prompt | ||
| let continue_prompt = unsafe { r_get_option::<String>("continue") }; |
There was a problem hiding this comment.
Missing lock when accessing the prompt global options
| // Include HTML representation of data.frame | ||
| // TODO: Do we need to hold the R lock here? | ||
| let value = unsafe { Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")) }; | ||
| if r_is_data_frame(value) { | ||
| match Kernel::to_html(value) { | ||
| Ok(html) => data.insert("text/html".to_string(), json!(html)), | ||
| Err(error) => { | ||
| error!("{:?}", error); | ||
| None | ||
| }, | ||
| }; | ||
| r_lock! { | ||
| let value = Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")); | ||
|
|
||
| // Include HTML representation of data.frame | ||
| if r_is_data_frame(value) { | ||
| match Kernel::to_html(value) { | ||
| Ok(html) => data.insert("text/html".to_string(), json!(html)), | ||
| Err(error) => { | ||
| error!("{:?}", error); | ||
| None | ||
| }, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing lock when finishing a code execution request
crates/harp/harp-macros/src/lib.rs
Outdated
| let out = quote! { | ||
| #(#attrs)* #vis #sig { | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| std::assert!( | ||
| crate::lock::R_RUNTIME_LOCKED.load(std::sync::atomic::Ordering::Acquire), | ||
| #message | ||
| ); | ||
|
|
||
| #(#stmts)* | ||
| } | ||
| }; |
There was a problem hiding this comment.
General idea is to just smush in this debug assertion ahead of the other statements in the function body
https://stackoverflow.com/questions/66850967/procedural-attribute-macro-to-inject-code-at-the-beginning-of-a-function
crates/harp/src/api.rs
Outdated
| // | ||
| // | ||
|
|
||
| use libR_sys::*; |
There was a problem hiding this comment.
Eventually this should be the only place we import this
crates/harp/src/lock.rs
Outdated
| // Debug only tracking of whether or not the thread holds the lock, | ||
| // used by the intermediate layer wrapping the R API | ||
| #[cfg(debug_assertions)] | ||
| pub static R_RUNTIME_LOCKED: std::sync::atomic::AtomicBool = | ||
| std::sync::atomic::AtomicBool::new(false); |
There was a problem hiding this comment.
An atomic boolean that gets set to true once we hold the lock and false right before we release it, so when the callback itself is executed it will see that this is true.
I'm not actually sure it needs to be atomic since we already have a mutex lock through guard? But someone else would have to weigh in here.
| let actual = unsafe { Rf_xlength(object) as usize }; | ||
| let actual = r_length(object) as usize; |
There was a problem hiding this comment.
Should allow us to remove some unsafes and begin to start using unsafe as it is intended in Rust
DavisVaughan
force-pushed
the
feature/r-lock-enforcement
branch
from
a09211e
to
9edf3e1
Compare
DavisVaughan
force-pushed
the
feature/r-lock-enforcement
branch
from
f352bdb
to
6f71e99
Compare
|
We can come back to this in the future but there is no need to keep it open right now as it isn't super high priority for alpha and I doubt we use this exact approach, but it will be a useful reference |
"Fixed" just enough of the missing
r_lock!s to get tests passing, but interactive usage could reveal more