Repository: mitsuhiko/minijinja (language: rust)

Issue:
Rendering gets stuck when invoked concurrently via minijinja-py
## Description

When rendering is invoked from parallel threads, it hangs and never returns.
I couldn't find anything about thread safety in the documentation, so it's possible that it's not even intended to be supported, in which case this might not be considered a bug.
A use case for running template rendering concurrently such as in the example below, is when the parent process doesn't know what the jobs that it's parallelizing are doing, and so it expects them to at least run correctly, even if not benefit from parallelization.

## Reproduction steps

```python
from time import time
from minijinja import Environment
from concurrent.futures import ThreadPoolExecutor

def busy_wait(value, seconds: float):
    start = time()
    while time() - start < seconds:
        continue
    return value

env = Environment(filters={'busy_wait': busy_wait})
executor = ThreadPoolExecutor()

for _ in range(2):
    executor.submit("{{ 'something' | busy_wait(1) }}")  # This hangs, even without waiting for the threads' results

```

The following workaround works:
```python
...

orig_render_str = env.render_str

def render_str(template: str):
    with lock:
        return orig_render_str(template)

env.render_str = render_str

futures = []
for _ in range(2):
    futures.append(executor.submit("{{ 'something' | busy_wait(1) }}"))

_ = [future.result() for future in futures]
```

Additional helpful information:

    Version of minijinja: 2.7.0
    Version of rustc: N/A
    Operating system and version: MacOS M1 Max Seqouia

## What did you expect
The `render_str` function is expected to not hang. Even if it means using an internal lock and not actually running in parallel, the program shouldn't get stuck.


Discussion / hints:
I think what is happening here is that the GIL is held too long and then we deadlock.  Will need to look into that.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8062b702..d355f335 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,8 @@ All notable changes to MiniJinja are documented here.
   Python binding not to compare correctly.  #707
 - Fixed a bug where `undeclared_variables` would incorrectly handle
   variables referenced by macros.  #714
+- Fixed a deadlock in the Python binding when multiple threads were
+  rendering from the same environment at once.  #717
 
 ## 2.8.0
 
diff --git a/Cargo.lock b/Cargo.lock
index a5672827..cec3be37 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1972,7 +1972,6 @@ name = "minijinja-py"
 version = "2.7.0"
 dependencies = [
  "minijinja",
- "once_cell",
  "pyo3",
 ]
 
diff --git a/Cargo.toml b/Cargo.toml
index 3bf21432..1ba7e9d1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -9,9 +9,6 @@ strip = true
 [profile.dev.package.insta]
 opt-level = 3
 
-[profile.dev.package.similar]
-opt-level = 3
-
 # The profile that 'cargo dist' will build with
 [profile.dist]
 inherits = "release"
diff --git a/minijinja-py/Cargo.toml b/minijinja-py/Cargo.toml
index 1457a72b..0ac63a1f 100644
--- a/minijinja-py/Cargo.toml
+++ b/minijinja-py/Cargo.toml
@@ -10,5 +10,4 @@ crate-type = ["cdylib"]
 
 [dependencies]
 minijinja = { version = "2.7.0", path = "../minijinja", features = ["loader", "json", "urlencode", "fuel", "preserve_order", "speedups", "custom_syntax", "loop_controls", "internal_safe_search"] }
-once_cell = "1.17.0"
 pyo3 = { version = "0.23.4", features = ["extension-module", "serde", "abi3-py38"] }
diff --git a/minijinja-py/README.md b/minijinja-py/README.md
index e26660e9..5fc1b740 100644
--- a/minijinja-py/README.md
+++ b/minijinja-py/README.md
@@ -173,6 +173,13 @@ Here is what this means for some basic types:
   However methods are disambiugated so `foo.items()` works and will correctly call
   the method in all cases.
 
+## Threading
+
+MiniJinja's Python bindin is thread-safe but it uses locks internally on the
+environment.  In particular only one thread can render a template from the same
+environment at the time.  If you want to render templates from multiple threads
+you should be creating a new environment for each thread.
+
 ## Sponsor
 
 If you like the project and find it useful you can [become a
diff --git a/minijinja-py/src/environment.rs b/minijinja-py/src/environment.rs
index d0317c38..4aba8329 100644
--- a/minijinja-py/src/environment.rs
+++ b/minijinja-py/src/environment.rs
@@ -2,7 +2,7 @@ use std::borrow::Cow;
 use std::collections::HashSet;
 use std::ffi::c_void;
 use std::sync::atomic::{AtomicBool, AtomicPtr, Ordering};
-use std::sync::Mutex;
+use std::sync::{Arc, Mutex};
 
 use minijinja::syntax::SyntaxConfig;
 use minijinja::value::{Rest, Value};
@@ -104,7 +104,7 @@ struct Inner {
 /// Represents a MiniJinja environment.
 #[pyclass(subclass, module = "minijinja._lowlevel")]
 pub struct Environment {
-    inner: Mutex<Inner>,
+    inner: Arc<Mutex<Inner>>,
     reload_before_render: AtomicBool,
 }
 
@@ -113,14 +113,14 @@ impl Environment {
     #[new]
     fn py_new() -> PyResult<Self> {
         Ok(Environment {
-            inner: Mutex::new(Inner {
+            inner: Arc::new(Mutex::new(Inner {
                 env: minijinja::Environment::new(),
                 loader: None,
                 auto_escape_callback: None,
                 finalizer_callback: None,
                 path_join_callback: None,
                 syntax: None,
-            }),
+            })),
             reload_before_render: AtomicBool::new(false),
         })
     }
@@ -299,13 +299,17 @@ impl Environment {
     /// Note that because this interface in MiniJinja is infallible, the callback is
     /// not able to raise an error.
     #[setter]
-    pub fn set_auto_escape_callback(&self, callback: &Bound<'_, PyAny>) -> PyResult<()> {
+    pub fn set_auto_escape_callback(
+        &self,
+        py: Python<'_>,
+        callback: &Bound<'_, PyAny>,
+    ) -> PyResult<()> {
         if !callback.is_callable() {
             return Err(PyRuntimeError::new_err("expected callback"));
         }
         let callback: Py<PyAny> = callback.clone().unbind();
         let mut inner = self.inner.lock().unwrap();
-        inner.auto_escape_callback = Python::with_gil(|py| Some(callback.clone_ref(py)));
+        inner.auto_escape_callback = Some(callback.clone_ref(py));
         inner
             .env
             .set_auto_escape_callback(move |name: &str| -> AutoEscape {
@@ -356,15 +360,13 @@ impl Environment {
     ///
     /// A finalizer is called before a value is rendered to customize it.
     #[setter]
-    pub fn set_finalizer(&self, callback: &Bound<'_, PyAny>) -> PyResult<()> {
+    pub fn set_finalizer(&self, py: Python<'_>, callback: &Bound<'_, PyAny>) -> PyResult<()> {
         if !callback.is_callable() {
             return Err(PyRuntimeError::new_err("expected callback"));
         }
         let callback: Py<PyAny> = callback.clone().unbind();
         let mut inner = self.inner.lock().unwrap();
-        Python::with_gil(|py| {
-            inner.finalizer_callback = Some(callback.clone_ref(py));
-        });
+        inner.finalizer_callback = Some(callback.clone_ref(py));
         inner.env.set_formatter(move |output, state, value| {
             Python::with_gil(|py| -> Result<(), Error> {
                 let maybe_new_value = bind_state(state, || -> Result<_, Error> {
@@ -407,7 +409,7 @@ impl Environment {
     /// template exists the source code of the template should be returned a string,
     /// otherwise `None` can be used to indicate that the template does not exist.
     #[setter]
-    pub fn set_loader(&self, callback: Option<&Bound<'_, PyAny>>) -> PyResult<()> {
+    pub fn set_loader(&self, py: Python<'_>, callback: Option<&Bound<'_, PyAny>>) -> PyResult<()> {
         let callback = match callback {
             None => None,
             Some(callback) => {
@@ -418,9 +420,7 @@ impl Environment {
             }
         };
         let mut inner = self.inner.lock().unwrap();
-        Python::with_gil(|py| {
-            inner.loader = callback.as_ref().map(|x| x.clone_ref(py));
-        });
+        inner.loader = callback.as_ref().map(|x| x.clone_ref(py));
 
         if let Some(callback) = callback {
             inner.env.set_loader(move |name| {
@@ -454,15 +454,17 @@ impl Environment {
 
     /// Sets a new path join callback.
     #[setter]
-    pub fn set_path_join_callback(&self, callback: &Bound<'_, PyAny>) -> PyResult<()> {
+    pub fn set_path_join_callback(
+        &self,
+        py: Python<'_>,
+        callback: &Bound<'_, PyAny>,
+    ) -> PyResult<()> {
         if !callback.is_callable() {
             return Err(PyRuntimeError::new_err("expected callback"));
         }
         let callback: Py<PyAny> = callback.clone().unbind();
         let mut inner = self.inner.lock().unwrap();
-        Python::with_gil(|py| {
-            inner.path_join_callback = Some(callback.clone_ref(py));
-        });
+        inner.path_join_callback = Some(callback.clone_ref(py));
         inner.env.set_path_join_callback(move |name, parent| {
             Python::with_gil(|py| {
                 let callback = callback.bind(py);
@@ -669,13 +671,16 @@ impl Environment {
         if slf.reload_before_render.load(Ordering::Relaxed) {
             slf.reload(py)?;
         }
+        let ctx = ctx
+            .map(|ctx| Value::from_object(DynamicObject::new(ctx.as_any().clone().unbind())))
+            .unwrap_or_else(|| context!());
         bind_environment(slf.as_ptr(), || {
-            let inner = slf.inner.lock().unwrap();
-            let tmpl = inner.env.get_template(template_name).map_err(to_py_error)?;
-            let ctx = ctx
-                .map(|ctx| Value::from_object(DynamicObject::new(ctx.as_any().clone().unbind())))
-                .unwrap_or_else(|| context!());
-            tmpl.render(ctx).map_err(to_py_error)
+            let inner = slf.inner.clone();
+            py.allow_threads(move || {
+                let inner = inner.lock().unwrap();
+                let tmpl = inner.env.get_template(template_name).map_err(to_py_error)?;
+                tmpl.render(ctx).map_err(to_py_error)
+            })
         })
     }
 
@@ -722,6 +727,7 @@ impl Environment {
     #[pyo3(signature = (source, name=None, /, **ctx))]
     pub fn render_str(
         slf: PyRef<'_, Self>,
+        py: Python<'_>,
         source: &str,
         name: Option<&str>,
         ctx: Option<&Bound<'_, PyDict>>,
@@ -730,12 +736,15 @@ impl Environment {
             let ctx = ctx
                 .map(|ctx| Value::from_object(DynamicObject::new(ctx.as_any().clone().unbind())))
                 .unwrap_or_else(|| context!());
-            slf.inner
-                .lock()
-                .unwrap()
-                .env
-                .render_named_str(name.unwrap_or("<string>"), source, ctx)
-                .map_err(to_py_error)
+            let inner = slf.inner.clone();
+            py.allow_threads(move || {
+                inner
+                    .lock()
+                    .unwrap()
+                    .env
+                    .render_named_str(name.unwrap_or("<string>"), source, ctx)
+                    .map_err(to_py_error)
+            })
         })
     }
 
@@ -743,34 +752,39 @@ impl Environment {
     #[pyo3(signature = (expression, /, **ctx))]
     pub fn eval_expr(
         slf: PyRef<'_, Self>,
+        py: Python<'_>,
         expression: &str,
         ctx: Option<&Bound<'_, PyDict>>,
     ) -> PyResult<Py<PyAny>> {
         bind_environment(slf.as_ptr(), || {
-            let inner = slf.inner.lock().unwrap();
-            let expr = inner
-                .env
-                .compile_expression(expression)
-                .map_err(to_py_error)?;
+            let inner = slf.inner.clone();
             let ctx = ctx
                 .map(|ctx| Value::from_object(DynamicObject::new(ctx.as_any().clone().unbind())))
                 .unwrap_or_else(|| context!());
-            to_python_value(expr.eval(ctx).map_err(to_py_error)?)
+            py.allow_threads(move || {
+                let inner = inner.lock().unwrap();
+                let expr = inner
+                    .env
+                    .compile_expression(expression)
+                    .map_err(to_py_error)?;
+                to_python_value(expr.eval(ctx).map_err(to_py_error)?)
+            })
         })
     }
 }
 
-pub fn with_environment<R, F: FnOnce(Py<Environment>) -> PyResult<R>>(f: F) -> PyResult<R> {
-    Python::with_gil(|py| {
-        CURRENT_ENV.with(|handle| {
-            let ptr = handle.load(Ordering::Relaxed) as *mut _;
-            match unsafe { Py::<Environment>::from_borrowed_ptr_or_opt(py, ptr) } {
-                Some(env) => f(env),
-                None => Err(PyRuntimeError::new_err(
-                    "environment cannot be used outside of template render",
-                )),
-            }
-        })
+pub fn with_environment<R, F: FnOnce(Py<Environment>) -> PyResult<R>>(
+    py: Python<'_>,
+    f: F,
+) -> PyResult<R> {
+    CURRENT_ENV.with(|handle| {
+        let ptr = handle.load(Ordering::Relaxed) as *mut _;
+        match unsafe { Py::<Environment>::from_borrowed_ptr_or_opt(py, ptr) } {
+            Some(env) => f(env),
+            None => Err(PyRuntimeError::new_err(
+                "environment cannot be used outside of template render",
+            )),
+        }
     })
 }
 
diff --git a/minijinja-py/src/error_support.rs b/minijinja-py/src/error_support.rs
index 3d3f5040..00dde05d 100644
--- a/minijinja-py/src/error_support.rs
+++ b/minijinja-py/src/error_support.rs
@@ -2,12 +2,12 @@ use std::any::Any;
 use std::cell::RefCell;
 
 use minijinja::{Error, ErrorKind};
-use once_cell::sync::OnceCell;
 use pyo3::ffi::PyErr_WriteUnraisable;
 use pyo3::prelude::*;
+use pyo3::sync::GILOnceCell;
 use pyo3::types::PyTuple;
 
-static TEMPLATE_ERROR: OnceCell<Py<PyAny>> = OnceCell::new();
+static TEMPLATE_ERROR: GILOnceCell<Py<PyAny>> = GILOnceCell::new();
 
 thread_local! {
     static STASHED_ERROR: RefCell<Option<PyErr>> = const { RefCell::new(None) };
@@ -97,7 +97,7 @@ pub fn report_unraisable(py: Python<'_>, err: PyErr) {
 
 fn make_error(err: Error) -> PyErr {
     Python::with_gil(|py| {
-        let template_error: &Py<PyAny> = TEMPLATE_ERROR.get_or_init(|| {
+        let template_error: &Py<PyAny> = TEMPLATE_ERROR.get_or_init(py, || {
             let module = py.import("minijinja._internal").unwrap();
             let err = module.getattr("make_error").unwrap();
             err.into()
diff --git a/minijinja-py/src/state.rs b/minijinja-py/src/state.rs
index 6bdb4f0a..7bb835d8 100644
--- a/minijinja-py/src/state.rs
+++ b/minijinja-py/src/state.rs
@@ -19,8 +19,8 @@ pub struct StateRef;
 impl StateRef {
     /// Returns a reference to the environment.
     #[getter]
-    pub fn get_env(&self) -> PyResult<Py<Environment>> {
-        with_environment(Ok)
+    pub fn get_env(&self, py: Python<'_>) -> PyResult<Py<Environment>> {
+        with_environment(py, Ok)
     }
 
     /// Returns the name of the template.
diff --git a/minijinja-py/src/typeconv.rs b/minijinja-py/src/typeconv.rs
index 04325e39..629a0802 100644
--- a/minijinja-py/src/typeconv.rs
+++ b/minijinja-py/src/typeconv.rs
@@ -6,8 +6,8 @@ use std::sync::{Arc, Mutex};
 use minijinja::value::{DynObject, Enumerator, Object, ObjectRepr, Value, ValueKind};
 use minijinja::{AutoEscape, Error, State};
 
-use once_cell::sync::OnceCell;
 use pyo3::pybacked::PyBackedStr;
+use pyo3::sync::GILOnceCell;
 use pyo3::types::{PyDict, PyList, PySequence, PyTuple};
 use pyo3::{prelude::*, IntoPyObjectExt};
 
@@ -15,7 +15,7 @@ use crate::error_support::{to_minijinja_error, to_py_error};
 use crate::state::{bind_state, StateRef};
 
 static AUTO_ESCAPE_CACHE: Mutex<BTreeMap<String, AutoEscape>> = Mutex::new(BTreeMap::new());
-static MARK_SAFE: OnceCell<Py<PyAny>> = OnceCell::new();
+static MARK_SAFE: GILOnceCell<Py<PyAny>> = GILOnceCell::new();
 
 fn is_safe_attr(name: &str) -> bool {
     !name.starts_with('_')
@@ -187,7 +187,7 @@ pub fn to_python_value(value: Value) -> PyResult<Py<PyAny>> {
 }
 
 fn mark_string_safe(py: Python<'_>, value: &str) -> PyResult<Py<PyAny>> {
-    let mark_safe: &Py<PyAny> = MARK_SAFE.get_or_try_init::<_, PyErr>(|| {
+    let mark_safe: &Py<PyAny> = MARK_SAFE.get_or_try_init::<_, PyErr>(py, || {
         let module = py.import("minijinja._internal")?;
         Ok(module.getattr("mark_safe")?.into())
     })?;


Test patch:
diff --git a/minijinja-py/tests/test_basic.py b/minijinja-py/tests/test_basic.py
index 999f6fe5..e9127032 100644
--- a/minijinja-py/tests/test_basic.py
+++ b/minijinja-py/tests/test_basic.py
@@ -428,3 +428,26 @@ class X:
         match="invalid operation: failed to sort: user-provided comparison function does not correctly implement a total order",
     ):
         env.eval_expr("values|sort", values=values)
+
+
+def test_threading_interactions():
+    from time import time
+    from concurrent.futures import ThreadPoolExecutor
+
+    done = []
+
+    def busy_wait(value, seconds: float):
+        start = time()
+        while time() - start < seconds:
+            continue
+        done.append(value)
+        return value
+
+    env = Environment(filters={"busy_wait": busy_wait})
+    executor = ThreadPoolExecutor()
+
+    for _ in range(4):
+        executor.submit(lambda: env.render_str("{{ 'something' | busy_wait(0.1) }}"))
+
+    executor.shutdown(wait=True)
+    assert done == ["something"] * 4