From 35fb0638f4f56238d6a8f553ec63ffbf9ab16d35 Mon Sep 17 00:00:00 2001 From: makspll Date: Sat, 22 Feb 2025 22:03:32 +0000 Subject: [PATCH 1/3] fix: functions not releasing accesses correctly on error --- .../src/bindings/function/script_function.rs | 88 ++++++++++++------- .../src/bindings/world.rs | 18 +++- 2 files changed, 72 insertions(+), 34 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 3249a40dbf..47481263c9 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 @@ -592,39 +592,42 @@ 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::function_call_error(FunctionError::ArgCountMismatch{ - expected: expected_arg_count, - received: 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::function_call_error(FunctionError::ArgCountMismatch{ + expected: expected_arg_count, + received: 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(); @@ -704,6 +707,27 @@ 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), ScriptValue::from(2)], + 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 9e4c11aba8..cab2369bd9 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(()) From e0a3f8a6e7fdca653f2bf3f0a359f7c343ed5f55 Mon Sep 17 00:00:00 2001 From: makspll Date: Sat, 22 Feb 2025 22:07:32 +0000 Subject: [PATCH 2/3] drive error deeper in test --- .../src/bindings/function/script_function.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 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 47481263c9..feb2adcfa5 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 @@ -716,10 +716,8 @@ mod test { let script_function = fn_.into_dynamic_script_function().with_name("my_fn"); with_local_world(|| { - let out = script_function.call( - vec![ScriptValue::from(1), ScriptValue::from(2)], - FunctionCallContext::default(), - ); + let out = + script_function.call(vec![ScriptValue::from(1)], FunctionCallContext::default()); assert!(out.is_err()); let world = FunctionCallContext::default().world().unwrap(); From 08f7695338a168045d4e6cf4a6f0aac45ec8c561 Mon Sep 17 00:00:00 2001 From: makspll Date: Sun, 23 Feb 2025 00:34:04 +0000 Subject: [PATCH 3/3] reduce job count further --- crates/xtask/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index c1f33e1856..0a36eb4bbc 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,