From 7a251c093cc450db938e891dae3c18a0bc25b4ba Mon Sep 17 00:00:00 2001 From: Maksymilian Mozolewski Date: Sun, 23 Feb 2025 01:11:07 +0000 Subject: [PATCH] fix: functions not releasing accesses correctly on error (#315) --- .../src/bindings/function/script_function.rs | 80 ++++++++++++------- .../src/bindings/world.rs | 18 ++++- crates/xtask/src/main.rs | 2 +- 3 files changed, 68 insertions(+), 32 deletions(-) diff --git a/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs b/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs index eb3b040e..bdb309d9 100644 --- a/crates/bevy_mod_scripting_core/src/bindings/function/script_function.rs +++ b/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 = 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| 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(); diff --git a/crates/bevy_mod_scripting_core/src/bindings/world.rs b/crates/bevy_mod_scripting_core/src/bindings/world.rs index 9e4c11ab..cab2369b 100644 --- a/crates/bevy_mod_scripting_core/src/bindings/world.rs +++ b/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>( + &self, + f: F, + ) -> Result { + 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(()) diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index c1f33e18..0a36eb4b 100644 --- a/crates/xtask/src/main.rs +++ b/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,