Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make extendr not thread-safe #674

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .cargo/config.toml
@@ -1,2 +1,5 @@
[env]
RUST_TEST_THREADS = "1"

[alias]
extendr = "run --package xtask --"
24 changes: 18 additions & 6 deletions .github/workflows/test.yml
Expand Up @@ -129,12 +129,18 @@ jobs:

New-Item -Path .cargo -ItemType Directory -Force
$pwd_slash = echo "${PWD}" | % {$_ -replace '\\','/'}

# assumes that there already are a `.cargo/config.toml`-file, with `[env]` block
$cargoConfig = Get-Content .cargo/config.toml
$envIndex = $cargoConfig | Select-String -Pattern "\[env\]" | Select-Object -First 1 -ExpandProperty LineNumber
$envIndex -= 1
$includeLine = "LIBRARY_PATH = `"${pwd_slash}/libgcc_mock`""
$cargoConfig = $cargoConfig[0..$envIndex] + $includeLine + $cargoConfig[($envIndex+1)..($cargoConfig.Count-1)]
$cargoConfig | Set-Content .cargo/config.toml -Encoding utf8

@"
[target.x86_64-pc-windows-gnu]
linker = "x86_64-w64-mingw32.static.posix-gcc.exe"

[env]
LIBRARY_PATH = "${pwd_slash}/libgcc_mock"
"@ | Out-File -FilePath .cargo/config.toml -Encoding utf8 -Append ;
env:
RUST_TOOLCHAIN: ${{ matrix.config.rust-version }}
Expand Down Expand Up @@ -366,12 +372,18 @@ jobs:

New-Item -Path .cargo -ItemType Directory -Force
$pwd_slash = echo "${PWD}" | % {$_ -replace '\\','/'}

# assumes that there already are a `.cargo/config.toml`-file, with `[env]` block
$cargoConfig = Get-Content .cargo/config.toml
$envIndex = $cargoConfig | Select-String -Pattern "\[env\]" | Select-Object -First 1 -ExpandProperty LineNumber
$envIndex -= 1
$includeLine = "LIBRARY_PATH = `"${pwd_slash}/libgcc_mock`""
$cargoConfig = $cargoConfig[0..$envIndex] + $includeLine + $cargoConfig[($envIndex+1)..($cargoConfig.Count-1)]
$cargoConfig | Set-Content .cargo/config.toml -Encoding utf8

@"
[target.x86_64-pc-windows-gnu]
linker = "x86_64-w64-mingw32.static.posix-gcc.exe"

[env]
LIBRARY_PATH = "${pwd_slash}/libgcc_mock"
"@ | Out-File -FilePath .cargo/config.toml -Encoding utf8 -Append ;
env:
RUST_TARGETS: ${{ join(matrix.config.rust-targets, ',') }}
Expand Down
18 changes: 9 additions & 9 deletions extendr-api/src/functions.rs
Expand Up @@ -66,7 +66,7 @@ pub fn global_function<K: Into<Robj>>(key: K) -> Result<Robj> {
/// ```
pub fn find_namespace<K: Into<Robj>>(key: K) -> Result<Environment> {
let key = key.into();
let res = single_threaded(|| call!(".getNamespace", key.clone()));
let res = call!(".getNamespace", key.clone());
if let Ok(res) = res {
Ok(res.try_into()?)
} else {
Expand Down Expand Up @@ -116,10 +116,10 @@ pub fn empty_env() -> Environment {
/// ```
#[cfg(use_r_newenv)]
pub fn new_env(parent: Environment, hash: bool, capacity: i32) -> Environment {
single_threaded(|| unsafe {
unsafe {
let env = R_NewEnv(parent.robj.get(), hash as i32, capacity);
Robj::from_sexp(env).try_into().unwrap()
})
}
}

// R_NewEnv is available as of R 4.1.0. For the older version, we call an R function `new.env()`.
Expand Down Expand Up @@ -207,7 +207,7 @@ pub fn blank_scalar_string() -> Robj {
/// }
/// ```
pub fn parse(code: &str) -> Result<Expressions> {
single_threaded(|| unsafe {
unsafe {
use libR_sys::*;
let mut status = 0_u32;
let status_ptr = &mut status as _;
Expand All @@ -217,7 +217,7 @@ pub fn parse(code: &str) -> Result<Expressions> {
1 => parsed.try_into(),
_ => Err(Error::ParseError(code.into())),
}
})
}
}

/// Parse a string into an R executable object and run it.
Expand All @@ -230,7 +230,7 @@ pub fn parse(code: &str) -> Result<Expressions> {
/// }
/// ```
pub fn eval_string(code: &str) -> Result<Robj> {
single_threaded(|| {
{
let expr = parse(code)?;
let mut res = Robj::from(());
if let Some(expr) = expr.as_expressions() {
Expand All @@ -239,7 +239,7 @@ pub fn eval_string(code: &str) -> Result<Robj> {
}
}
Ok(res)
})
}
}

/// Parse a string into an R executable object and run it using
Expand All @@ -254,7 +254,7 @@ pub fn eval_string(code: &str) -> Result<Robj> {
/// }
/// ```
pub fn eval_string_with_params(code: &str, values: &[&Robj]) -> Result<Robj> {
single_threaded(|| {
{
let env = Environment::new_with_parent(global_env());
for (i, &v) in values.iter().enumerate() {
let key = Symbol::from_string(format!("param.{}", i));
Expand All @@ -270,7 +270,7 @@ pub fn eval_string_with_params(code: &str, values: &[&Robj]) -> Result<Robj> {
}

Ok(res)
})
}
}

/// Find a function or primitive that may be in a namespace.
Expand Down
4 changes: 2 additions & 2 deletions extendr-api/src/iter.rs
Expand Up @@ -57,7 +57,7 @@ impl StrIter {

// Get a string reference from a CHARSXP
fn str_from_strsxp<'a>(sexp: SEXP, index: isize) -> &'a str {
single_threaded(|| unsafe {
unsafe {
if index < 0 || index >= Rf_xlength(sexp) {
<&str>::na()
} else {
Expand All @@ -72,7 +72,7 @@ fn str_from_strsxp<'a>(sexp: SEXP, index: isize) -> &'a str {
<&str>::na()
}
}
})
}
}

impl Iterator for StrIter {
Expand Down
11 changes: 5 additions & 6 deletions extendr-api/src/lang_macros.rs
Expand Up @@ -3,7 +3,6 @@

use crate::robj::GetSexp;
use crate::robj::Robj;
use crate::single_threaded;
use libR_sys::*;

/// Convert a list of tokens to an array of tuples.
Expand Down Expand Up @@ -43,26 +42,26 @@ macro_rules! args {

#[doc(hidden)]
pub unsafe fn append_with_name(tail: SEXP, obj: Robj, name: &str) -> SEXP {
single_threaded(|| {
{
let cons = Rf_cons(obj.get(), R_NilValue);
SET_TAG(cons, crate::make_symbol(name));
SETCDR(tail, cons);
cons
})
}
}

#[doc(hidden)]
pub unsafe fn append(tail: SEXP, obj: Robj) -> SEXP {
single_threaded(|| {
{
let cons = Rf_cons(obj.get(), R_NilValue);
SETCDR(tail, cons);
cons
})
}
}

#[doc(hidden)]
pub unsafe fn make_lang(sym: &str) -> Robj {
Robj::from_sexp(single_threaded(|| Rf_lang1(crate::make_symbol(sym))))
Robj::from_sexp(Rf_lang1(crate::make_symbol(sym)))
}

/// Convert a list of tokens to an array of tuples.
Expand Down
8 changes: 4 additions & 4 deletions extendr-api/src/ownership.rs
Expand Up @@ -233,7 +233,7 @@ mod test {
#[test]
fn basic_test() {
test! {
single_threaded(|| unsafe {
unsafe {
{
let mut own = OWNERSHIP.lock().expect("lock failed");
own.check_objects();
Expand Down Expand Up @@ -289,14 +289,14 @@ mod test {
assert_eq!(own.ref_count(sexp2), 1);
}
Rf_unprotect(2);
});
};
}
}

#[test]
fn collection_test() {
test! {
single_threaded(|| unsafe {
unsafe {
{
let mut own = OWNERSHIP.lock().expect("protect failed");
own.check_objects();
Expand Down Expand Up @@ -330,7 +330,7 @@ mod test {
}

Rf_unprotect(1);
});
};
}
}
}
9 changes: 4 additions & 5 deletions extendr-api/src/robj/into_robj.rs
@@ -1,6 +1,5 @@
use super::*;
use crate::scalar::Scalar;
use crate::single_threaded;

mod repeat_into_robj;

Expand All @@ -9,13 +8,13 @@ pub(crate) fn str_to_character(s: &str) -> SEXP {
if s.is_na() {
R_NaString
} else {
single_threaded(|| {
{
Rf_mkCharLenCE(
s.as_ptr() as *const raw::c_char,
s.len() as i32,
cetype_t_CE_UTF8,
)
})
}
}
}
}
Expand Down Expand Up @@ -520,7 +519,7 @@ where
I: Sized,
I::Item: ToVectorValue,
{
single_threaded(|| unsafe {
unsafe {
// Length of the vector is known in advance.
let sexptype = I::Item::sexptype();
if sexptype != 0 {
Expand Down Expand Up @@ -570,7 +569,7 @@ where
} else {
Robj::from(())
}
})
}
}

/// Extensions to iterators for R objects including [RobjItertools::collect_robj()].
Expand Down
12 changes: 6 additions & 6 deletions extendr-api/src/robj/mod.rs
Expand Up @@ -220,10 +220,10 @@ impl Length for Robj {}

impl Robj {
pub fn from_sexp(sexp: SEXP) -> Self {
single_threaded(|| {
{
unsafe { ownership::protect(sexp) };
Robj { inner: sexp }
})
}
}

/// A ref of an robj can be constructed from a ref to a SEXP
Expand Down Expand Up @@ -703,15 +703,15 @@ pub trait Eval: GetSexp {
/// }
/// ```
fn eval_with_env(&self, env: &Environment) -> Result<Robj> {
single_threaded(|| unsafe {
unsafe {
let mut error: raw::c_int = 0;
let res = R_tryEval(self.get(), env.get(), &mut error as *mut raw::c_int);
if error != 0 {
Err(Error::EvalError(Robj::from_sexp(self.get())))
} else {
Ok(Robj::from_sexp(res))
}
})
}
}

/// Evaluate the expression and return NULL or an R object.
Expand Down Expand Up @@ -865,13 +865,13 @@ pub trait Attributes: Types + Length {
let value = value.into();
unsafe {
let sexp = self.get_mut();
single_threaded(|| {
{
catch_r_error(|| Rf_setAttrib(sexp, name.get(), value.get()))
// FIXME: there is no reason to re-wrap this, as this mutates
// the input `self`, and returns another pointer to the same
// object
.map(|_| Robj::from_sexp(sexp))
})
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions extendr-api/src/robj/operators.rs
Expand Up @@ -101,10 +101,10 @@ pub trait Operators: Rinternals {
/// ```
fn call(&self, args: Pairlist) -> Result<Robj> {
if self.is_function() {
single_threaded(|| unsafe {
unsafe {
let call = Robj::from_sexp(Rf_lcons(self.get(), args.get()));
call.eval()
})
}
} else {
Err(Error::ExpectedFunction(self.as_robj().clone()))
}
Expand Down