Repository: clarkmcc/cel-rust (language: rust)

Issue:
Support thread-safe program execution
I'm experimenting with writing a Python extension for this library using `pyo3` and running into issues when it comes down to concurrency. I'm not really well-versed in Rust, but I asked about it [here](https://github.com/PyO3/pyo3/discussions/3477). As far as I understand it boils down to using `Arc` instead of `Rc`.

I'm currently using a Python version of CEL interpreter, but its performance leaves a lot to be desired, so I'm looking for an alternatives. I use CEL for feature flags so we have multiple compiled expressions which are evaluated from different threads.

What are your thoughts about it? What would it take to make the interpreter thread-safe?

I'm willing to help, but my Rust knowledge if very very limited :)


Discussion / hints:
I have actually try replacing all `Rc` into `Arc` and it seems to be working and all tests seem to pass, so at the very least this is a working solution, but I don't know if there are any drawbacks.

Fix patch:
diff --git a/example/Cargo.toml b/example/Cargo.toml
index b399173..ef985af 100644
--- a/example/Cargo.toml
+++ b/example/Cargo.toml
@@ -19,4 +19,8 @@ path = "src/variables.rs"
 
 [[bin]]
 name = "functions"
-path = "src/functions.rs"
\ No newline at end of file
+path = "src/functions.rs"
+
+[[bin]]
+name = "threads"
+path = "src/threads.rs"
\ No newline at end of file
diff --git a/example/src/functions.rs b/example/src/functions.rs
index 8eb5276..497af9e 100644
--- a/example/src/functions.rs
+++ b/example/src/functions.rs
@@ -2,7 +2,7 @@
 use cel_interpreter::extractors::This;
 use cel_interpreter::{Context, ExecutionError, FunctionContext, Program, ResolveResult, Value};
 use chrono::{DateTime, Duration, FixedOffset};
-use std::rc::Rc;
+use std::sync::Arc;
 
 fn main() {
     let program = Program::compile("add(2, 3) == 5 && ''.isEmpty() && fail()").unwrap();
@@ -32,7 +32,7 @@ fn main() {
 /// ```skip
 /// "foo".isEmpty()
 /// ```
-fn is_empty(This(s): This<Rc<String>>) -> bool {
+fn is_empty(This(s): This<Arc<String>>) -> bool {
     s.is_empty()
 }
 
@@ -55,11 +55,11 @@ fn primitives(
     _b: u64,
     _c: f64,
     _d: bool,
-    _e: Rc<String>,
-    _f: Rc<Vec<u8>>,
+    _e: Arc<String>,
+    _f: Arc<Vec<u8>>,
     _g: Duration,
     _h: DateTime<FixedOffset>,
-    _i: Rc<Vec<Value>>,
+    _i: Arc<Vec<Value>>,
 ) -> Duration {
     Duration::zero()
 }
diff --git a/example/src/threads.rs b/example/src/threads.rs
new file mode 100644
index 0000000..e55b5d1
--- /dev/null
+++ b/example/src/threads.rs
@@ -0,0 +1,23 @@
+use cel_interpreter::{Context, Program};
+use std::thread::scope;
+
+fn main() {
+    let program = Program::compile("a + b").unwrap();
+
+    scope(|scope| {
+        scope.spawn(|| {
+            let mut context = Context::default();
+            context.add_variable("a", 1);
+            context.add_variable("b", 2);
+            let value = program.execute(&context).unwrap();
+            assert_eq!(value, 3.into());
+        });
+        scope.spawn(|| {
+            let mut context = Context::default();
+            context.add_variable("a", 2);
+            context.add_variable("b", 4);
+            let value = program.execute(&context).unwrap();
+            assert_eq!(value, 6.into());
+        });
+    });
+}
diff --git a/interpreter/README.md b/interpreter/README.md
index 9d5b088..5fd6376 100644
--- a/interpreter/README.md
+++ b/interpreter/README.md
@@ -51,3 +51,4 @@ Check out these other examples to learn how to use this library:
 - [Simple](../example/src/simple.rs) - A simple example of how to use the library.
 - [Variables](../example/src/variables.rs) - Passing variables and using them in your program.
 - [Functions](../example/src/functions.rs) - Defining and using custom functions in your program.
+- [Concurrent Execution](../example/src/threads.rs) - Executing the same program concurrently.
diff --git a/interpreter/src/functions.rs b/interpreter/src/functions.rs
index 717ac19..c0963e3 100644
--- a/interpreter/src/functions.rs
+++ b/interpreter/src/functions.rs
@@ -7,7 +7,7 @@ use crate::ExecutionError;
 use cel_parser::Expression;
 use chrono::{DateTime, Duration, FixedOffset};
 use std::convert::TryInto;
-use std::rc::Rc;
+use std::sync::Arc;
 
 type Result<T> = std::result::Result<T, ExecutionError>;
 
@@ -17,7 +17,7 @@ type Result<T> = std::result::Result<T, ExecutionError>;
 /// to variables, and the arguments to the function call.
 #[derive(Clone)]
 pub struct FunctionContext<'context> {
-    pub name: Rc<String>,
+    pub name: Arc<String>,
     pub this: Option<Value>,
     pub ptx: &'context Context<'context>,
     pub args: Vec<Expression>,
@@ -26,7 +26,7 @@ pub struct FunctionContext<'context> {
 
 impl<'context> FunctionContext<'context> {
     pub fn new(
-        name: Rc<String>,
+        name: Arc<String>,
         this: Option<Value>,
         ptx: &'context Context<'context>,
         args: Vec<Expression>,
@@ -152,7 +152,7 @@ pub fn string(ftx: &FunctionContext, This(this): This<Value>) -> Result<Value> {
         Value::Int(v) => Value::String(v.to_string().into()),
         Value::UInt(v) => Value::String(v.to_string().into()),
         Value::Float(v) => Value::String(v.to_string().into()),
-        Value::Bytes(v) => Value::String(Rc::new(String::from_utf8_lossy(v.as_slice()).into())),
+        Value::Bytes(v) => Value::String(Arc::new(String::from_utf8_lossy(v.as_slice()).into())),
         v => return Err(ftx.error(&format!("cannot convert {:?} to string", v))),
     })
 }
@@ -174,7 +174,7 @@ pub fn double(ftx: &FunctionContext, This(this): This<Value>) -> Result<Value> {
 /// ```cel
 /// "abc".startsWith("a") == true
 /// ```
-pub fn starts_with(This(this): This<Rc<String>>, prefix: Rc<String>) -> bool {
+pub fn starts_with(This(this): This<Arc<String>>, prefix: Arc<String>) -> bool {
     this.starts_with(prefix.as_str())
 }
 
@@ -226,7 +226,7 @@ pub fn map(
                 let value = ptx.resolve(&expr)?;
                 values.push(value);
             }
-            Value::List(Rc::new(values))
+            Value::List(Arc::new(values))
         }
         _ => return Err(this.error_expected_type(ValueType::List)),
     }
@@ -260,7 +260,7 @@ pub fn filter(
                     values.push(item.clone());
                 }
             }
-            Value::List(Rc::new(values))
+            Value::List(Arc::new(values))
         }
         _ => return Err(this.error_expected_type(ValueType::List)),
     }
@@ -324,13 +324,13 @@ pub fn all(
 /// - `1.5ms` parses as 1 millisecond and 500 microseconds
 /// - `1ns` parses as 1 nanosecond
 /// - `1.5ns` parses as 1 nanosecond (sub-nanosecond durations not supported)
-pub fn duration(value: Rc<String>) -> Result<Value> {
+pub fn duration(value: Arc<String>) -> Result<Value> {
     Ok(Value::Duration(_duration(value.as_str())?))
 }
 
 /// Timestamp parses the provided argument into a [`Value::Timestamp`] value.
 /// The
-pub fn timestamp(value: Rc<String>) -> Result<Value> {
+pub fn timestamp(value: Arc<String>) -> Result<Value> {
     Ok(Value::Timestamp(
         DateTime::parse_from_rfc3339(value.as_str())
             .map_err(|e| ExecutionError::function_error("timestamp", e.to_string().as_str()))?,
diff --git a/interpreter/src/lib.rs b/interpreter/src/lib.rs
index 69b2c54..d56bd44 100644
--- a/interpreter/src/lib.rs
+++ b/interpreter/src/lib.rs
@@ -2,7 +2,7 @@ extern crate core;
 
 use cel_parser::parse;
 use std::convert::TryFrom;
-use std::rc::Rc;
+use std::sync::Arc;
 use thiserror::Error;
 
 mod macros;
@@ -47,11 +47,11 @@ pub enum ExecutionError {
     /// Indicates that the script attempted to reference a key on a type that
     /// was missing the requested key.
     #[error("No such key: {0}")]
-    NoSuchKey(Rc<String>),
+    NoSuchKey(Arc<String>),
     /// Indicates that the script attempted to reference an undeclared variable
     /// method, or function.
     #[error("Undeclared reference to '{0}'")]
-    UndeclaredReference(Rc<String>),
+    UndeclaredReference(Arc<String>),
     /// Indicates that a function expected to be called as a method, or to be
     /// called with at least one parameter.
     #[error("Missing argument or target")]
@@ -63,11 +63,11 @@ pub enum ExecutionError {
 
 impl ExecutionError {
     pub fn no_such_key(name: &str) -> Self {
-        ExecutionError::NoSuchKey(Rc::new(name.to_string()))
+        ExecutionError::NoSuchKey(Arc::new(name.to_string()))
     }
 
     pub fn undeclared_reference(name: &str) -> Self {
-        ExecutionError::UndeclaredReference(Rc::new(name.to_string()))
+        ExecutionError::UndeclaredReference(Arc::new(name.to_string()))
     }
 
     pub fn invalid_argument_count(expected: usize, actual: usize) -> Self {
diff --git a/interpreter/src/magic.rs b/interpreter/src/magic.rs
index 30d5e62..8f89525 100644
--- a/interpreter/src/magic.rs
+++ b/interpreter/src/magic.rs
@@ -5,18 +5,18 @@ use cel_parser::Expression;
 use chrono::{DateTime, Duration, FixedOffset};
 use std::collections::HashMap;
 use std::marker::PhantomData;
-use std::rc::Rc;
+use std::sync::Arc;
 
 impl_conversions!(
     i64 => Value::Int,
     u64 => Value::UInt,
     f64 => Value::Float,
-    Rc<String> => Value::String,
-    Rc<Vec<u8>> => Value::Bytes,
+    Arc<String> => Value::String,
+    Arc<Vec<u8>> => Value::Bytes,
     bool => Value::Bool,
     Duration => Value::Duration,
     DateTime<FixedOffset> => Value::Timestamp,
-    Rc<Vec<Value>> => Value::List
+    Arc<Vec<Value>> => Value::List
 );
 
 impl From<i32> for Value {
@@ -52,7 +52,7 @@ pub trait IntoResolveResult {
 
 impl IntoResolveResult for String {
     fn into_resolve_result(self) -> ResolveResult {
-        Ok(Value::String(Rc::new(self)))
+        Ok(Value::String(Arc::new(self)))
     }
 }
 
@@ -80,7 +80,7 @@ pub(crate) trait FromContext<'a, 'context> {
 ///
 /// # Using `This`
 /// ```
-/// # use std::rc::Rc;
+/// # use std::sync::Arc;
 /// # use cel_interpreter::{Program, Context};
 /// use cel_interpreter::extractors::This;
 /// # let mut context = Context::default();
@@ -97,7 +97,7 @@ pub(crate) trait FromContext<'a, 'context> {
 /// # let value = program2.execute(&context).unwrap();
 /// # assert_eq!(value, true.into());
 ///
-/// fn starts_with(This(this): This<Rc<String>>, prefix: Rc<String>) -> bool {
+/// fn starts_with(This(this): This<Arc<String>>, prefix: Arc<String>) -> bool {
 ///     this.starts_with(prefix.as_str())
 /// }
 /// ```
@@ -168,7 +168,7 @@ where
 /// ) -> Result<Value>;
 /// ```
 #[derive(Clone)]
-pub struct Identifier(pub Rc<String>);
+pub struct Identifier(pub Arc<String>);
 
 impl<'a, 'context> FromContext<'a, 'context> for Identifier {
     fn from_context(ctx: &'a mut FunctionContext<'context>) -> Result<Self, ExecutionError>
@@ -198,7 +198,7 @@ impl From<Identifier> for String {
 }
 
 #[derive(Clone)]
-pub struct List(pub Rc<Vec<Value>>);
+pub struct List(pub Arc<Vec<Value>>);
 
 impl FromValue for List {
     fn from_value(value: &Value) -> Result<Self, ExecutionError>
@@ -233,7 +233,7 @@ impl FromValue for List {
 /// }
 /// ```
 #[derive(Clone)]
-pub struct Arguments(pub Rc<Vec<Value>>);
+pub struct Arguments(pub Arc<Vec<Value>>);
 
 impl<'a, 'context> FromContext<'a, 'context> for Arguments {
     fn from_context(ctx: &'a mut FunctionContext) -> Result<Self, ExecutionError>
diff --git a/interpreter/src/objects.rs b/interpreter/src/objects.rs
index c5d5ca2..3dd134c 100644
--- a/interpreter/src/objects.rs
+++ b/interpreter/src/objects.rs
@@ -10,6 +10,7 @@ use std::collections::HashMap;
 use std::convert::TryInto;
 use std::fmt::{Display, Formatter};
 use std::rc::Rc;
+use std::sync::Arc;
 
 #[derive(Debug, PartialEq, Clone)]
 pub struct Map {
@@ -27,7 +28,7 @@ pub enum Key {
     Int(i64),
     Uint(u64),
     Bool(bool),
-    String(Rc<String>),
+    String(Arc<String>),
 }
 
 /// Implement conversions from primitive types to [`Key`]
@@ -38,15 +39,15 @@ impl From<String> for Key {
     }
 }
 
-impl From<Rc<String>> for Key {
-    fn from(v: Rc<String>) -> Self {
+impl From<Arc<String>> for Key {
+    fn from(v: Arc<String>) -> Self {
         Key::String(v.clone())
     }
 }
 
 impl<'a> From<&'a str> for Key {
     fn from(v: &'a str) -> Self {
-        Key::String(Rc::new(v.into()))
+        Key::String(Arc::new(v.into()))
     }
 }
 
@@ -100,17 +101,17 @@ impl<K: Into<Key>, V: Into<Value>> From<HashMap<K, V>> for Map {
 
 #[derive(Debug, Clone, PartialEq)]
 pub enum Value {
-    List(Rc<Vec<Value>>),
+    List(Arc<Vec<Value>>),
     Map(Map),
 
-    Function(Rc<String>, Option<Box<Value>>),
+    Function(Arc<String>, Option<Box<Value>>),
 
     // Atoms
     Int(i64),
     UInt(u64),
     Float(f64),
-    String(Rc<String>),
-    Bytes(Rc<Vec<u8>>),
+    String(Arc<String>),
+    Bytes(Arc<Vec<u8>>),
     Bool(bool),
     Duration(Duration),
     Timestamp(DateTime<FixedOffset>),
diff --git a/parser/src/ast.rs b/parser/src/ast.rs
index 7dc5aad..50953e8 100644
--- a/parser/src/ast.rs
+++ b/parser/src/ast.rs
@@ -1,4 +1,4 @@
-use std::rc::Rc;
+use std::sync::Arc;
 
 #[derive(Debug, Eq, PartialEq, Clone)]
 pub enum RelationOp {
@@ -44,15 +44,15 @@ pub enum Expression {
     Map(Vec<(Expression, Expression)>),
 
     Atom(Atom),
-    Ident(Rc<String>),
+    Ident(Arc<String>),
 }
 
 #[derive(Debug, PartialEq, Clone)]
 pub enum Member {
-    Attribute(Rc<String>),
+    Attribute(Arc<String>),
     FunctionCall(Vec<Expression>),
     Index(Box<Expression>),
-    Fields(Vec<(Rc<String>, Expression)>),
+    Fields(Vec<(Arc<String>, Expression)>),
 }
 
 #[derive(Debug, PartialEq, Clone)]
@@ -60,8 +60,8 @@ pub enum Atom {
     Int(i64),
     UInt(u64),
     Float(f64),
-    String(Rc<String>),
-    Bytes(Rc<Vec<u8>>),
+    String(Arc<String>),
+    Bytes(Arc<Vec<u8>>),
     Bool(bool),
     Null,
 }
diff --git a/parser/src/cel.lalrpop b/parser/src/cel.lalrpop
index 283bb2e..9379331 100644
--- a/parser/src/cel.lalrpop
+++ b/parser/src/cel.lalrpop
@@ -1,5 +1,5 @@
 use crate::{RelationOp, ArithmeticOp, Expression, UnaryOp, Member, Atom};
-use std::rc::Rc;
+use std::sync::Arc;
 
 grammar;
 
@@ -73,7 +73,7 @@ pub Primary: Expression = {
     "(" <Expression> ")"
 }
 
-pub FieldInits: (Rc<String>, Expression) = {
+pub FieldInits: (Arc<String>, Expression) = {
     <Ident> ":" <Expression>
 }
 
@@ -138,19 +138,19 @@ Atom: Atom = {
     // the LALRPOP parser.
 
     // Double quoted string
-    r#""(\\.|[^"\n])*""# => Atom::String(<>[1..<>.len()-1].to_string().into()),
+    r#""(\\.|[^"\n])*""# => Atom::String(Arc::from(<>[1..<>.len()-1].to_string())),
     // r#""""(\\.|[^"{3}])*""""# => Atom::String(<>.to_string().into()),
 
     // Single quoted string
-    r#"'(\\.|[^'\n])*'"# => Atom::String(<>[1..<>.len()-1].to_string().into()),
+    r#"'(\\.|[^'\n])*'"# => Atom::String(Arc::from(<>[1..<>.len()-1].to_string())),
     // r#"'''(\\.|[^'{3}])*'''"# => Atom::String(<>.to_string().into()),
 
     // Double quoted bytes
-    r#"[bB]"(\\.|[^"\n])*""# => Atom::Bytes(Vec::from(<>[2..<>.len()-1].as_bytes()).into()),
+    r#"[bB]"(\\.|[^"\n])*""# => Atom::Bytes(Arc::from(<>[2..<>.len()-1].as_bytes().to_vec())),
     // r#"[bB]"""(\\.|[^"{3}])*""""# => Atom::Bytes(Vec::from(<>.as_bytes()).into()),
 
     // Single quoted bytes
-    r#"[bB]'(\\.|[^'\n])*'"# => Atom::Bytes(Vec::from(<>[2..<>.len()-1].as_bytes()).into()),
+    r#"[bB]'(\\.|[^'\n])*'"# => Atom::Bytes(Arc::from(<>[2..<>.len()-1].as_bytes().to_vec())),
     // r#"[bB]'''(\\.|[^'{3}])*'''"# => Atom::Bytes(Vec::from(<>.as_bytes()).into()),
 
     "true" => Atom::Bool(true),
@@ -158,6 +158,6 @@ Atom: Atom = {
     "null" => Atom::Null,
 };
 
-Ident: Rc<String> = {
-    r"[_a-zA-Z][_a-zA-Z0-9]*" => <>.to_string().into()
+Ident: Arc<String> = {
+    r"[_a-zA-Z][_a-zA-Z0-9]*" => Arc::from(<>.to_string())
 }
\ No newline at end of file


Test patch: