fix: functions not releasing accesses correctly on error (#315)

Author: makspll

crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs

@@ -589,36 +589,39 @@ macro_rules! impl_script_function {
 
                         $( let $context = caller_context; )?
                         let world = caller_context.world()?;
-                        world.begin_access_scope()?;
-                        let mut current_arg = 0;
-
-                        $(
-                            current_arg += 1;
-                            let $param = args.pop_front();
-                            let $param = match $param {
-                                Some($param) => $param,
-                                None => {
-                                    if let Some(default) = <$param>::default_value() {
-                                        default
-                                    } else {
-                                        return Err(InteropError::argument_count_mismatch(expected_arg_count,received_args_len));
-                                    }
-                                }
-                            };
-                            let $param = <$param>::from_script($param, world.clone())
-                                .map_err(|e| InteropError::function_arg_conversion_error(current_arg.to_string(), e))?;
-                        )*
-
-                        let ret = {
-                            let out = self( $( $context,)?  $( $param.into(), )* );
-                            $(
-                                let $out = out?;
-                                let out = $out;
-                            )?
-                            out.into_script(world.clone()).map_err(|e| InteropError::function_arg_conversion_error("return value".to_owned(), e))
+                        // Safety: we're not holding any references to the world, the arguments which might have aliased will always be dropped
+                        let ret: Result<ScriptValue, InteropError> = unsafe {
+                            world.with_access_scope(||{
+                                let mut current_arg = 0;
+
+                                $(
+                                    current_arg += 1;
+                                    let $param = args.pop_front();
+                                    let $param = match $param {
+                                        Some($param) => $param,
+                                        None => {
+                                            if let Some(default) = <$param>::default_value() {
+                                                default
+                                            } else {
+                                                return Err(InteropError::argument_count_mismatch(expected_arg_count,received_args_len));
+                                            }
+                                        }
+                                    };
+                                    let $param = <$param>::from_script($param, world.clone())
+                                        .map_err(|e| InteropError::function_arg_conversion_error(current_arg.to_string(), e))?;
+                                )*
+
+                                let ret = {
+                                    let out = self( $( $context,)?  $( $param.into(), )* );
+                                    $(
+                                        let $out = out?;
+                                        let out = $out;
+                                    )?
+                                    out.into_script(world.clone()).map_err(|e| InteropError::function_arg_conversion_error("return value".to_owned(), e))
+                                };
+                                ret
+                            })?
                         };
-                        // Safety: we're not holding any references to the world, the arguments which might have aliased have been dropped
-                        unsafe { world.end_access_scope()? };
                         ret
                     })();
                     let script_value: ScriptValue = res.into();
@@ -695,6 +698,25 @@ mod test {
         });
     }
 
+    #[test]
+    fn test_interrupted_call_releases_access_scope() {
+        #[derive(bevy::prelude::Component, Reflect)]
+        struct Comp;
+
+        let fn_ = |_a: crate::bindings::function::from::Mut<Comp>| 0usize;
+        let script_function = fn_.into_dynamic_script_function().with_name("my_fn");
+
+        with_local_world(|| {
+            let out =
+                script_function.call(vec![ScriptValue::from(1)], FunctionCallContext::default());
+
+            assert!(out.is_err());
+            let world = FunctionCallContext::default().world().unwrap();
+            // assert no access is held
+            assert!(world.list_accesses().is_empty());
+        });
+    }
+
     #[test]
     fn test_overloaded_script_function() {
         let mut registry = ScriptFunctionRegistry::default();

crates/bevy_mod_scripting_core/src/bindings/world.rs

@@ -123,8 +123,22 @@ impl<'w> WorldAccessGuard<'w> {
         self.0.cell.replace(None);
     }
 
+    /// Runs a closure within an isolated access scope, releasing leftover accesses, should only be used in a single-threaded context.
+    ///
+    /// Safety:
+    /// - The caller must ensure it's safe to release any potentially locked accesses.
+    pub(crate) unsafe fn with_access_scope<O, F: FnOnce() -> O>(
+        &self,
+        f: F,
+    ) -> Result<O, InteropError> {
+        self.begin_access_scope()?;
+        let o = f();
+        unsafe { self.end_access_scope()? };
+        Ok(o)
+    }
+
     /// Begins a new access scope. Currently this simply throws an erorr if there are any accesses held. Should only be used in a single-threaded context
-    pub(crate) fn begin_access_scope(&self) -> Result<(), InteropError> {
+    fn begin_access_scope(&self) -> Result<(), InteropError> {
         if self.0.accesses.count_accesses() != 0 {
             return Err(InteropError::invalid_access_count(self.0.accesses.count_accesses(), 0, "When beginning access scope, presumably for a function call, some accesses are still held".to_owned()));
         }
@@ -133,7 +147,7 @@ impl<'w> WorldAccessGuard<'w> {
     }
 
     /// Ends the access scope, releasing all accesses. Should only be used in a single-threaded context
-    pub(crate) unsafe fn end_access_scope(&self) -> Result<(), InteropError> {
+    unsafe fn end_access_scope(&self) -> Result<(), InteropError> {
         self.0.accesses.release_all_accesses();
 
         Ok(())

crates/xtask/src/main.rs

@@ -1372,7 +1372,7 @@ impl Xtasks {
                 .clone()
                 .with_coverage()
                 // github actions has been throwing a lot of OOM SIGTERM's lately
-                .with_max_jobs(4),
+                .with_max_jobs(2),
             subcmd: Xtasks::Test {
                 name: None,
                 package: None,