actually release access after function calls

Author: makspll

crates/bevy_mod_scripting_core/src/bindings/access_map.rs

@@ -279,16 +279,14 @@ impl AccessMap {
             .collect()
     }
 
-    pub fn count_thread_acceesses(&self) -> usize {
-        self.individual_accesses
-            .iter()
-            .filter(|e| {
-                e.value()
-                    .read_by
-                    .iter()
-                    .any(|o| o.id == std::thread::current().id())
-            })
-            .count()
+    pub fn count_accesses(&self) -> usize {
+        self.individual_accesses.len()
+    }
+
+    pub fn release_all_accesses(&self) {
+        self.individual_accesses.clear();
+        self.global_lock
+            .store(false, std::sync::atomic::Ordering::Relaxed);
     }
 
     pub fn access_location<K: AccessMapKey>(

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

@@ -144,20 +144,25 @@ macro_rules! impl_script_function {
                     let res: Result<ScriptValue, InteropError> = (|| {
                         $( let $callback = world.clone(); )?
                         let world = world.try_read()?;
-                        // TODO: snapshot the accesses and release them after
-                        #[allow(unused_mut,unused_variables)]
-                        let mut current_arg = 0;
-                        $(
-                            current_arg += 1;
-                            let $param = <$param>::from_script($param, world.clone())
-                                .map_err(|e| InteropError::function_arg_conversion_error(current_arg.to_string(), e))?;
-                        )*
-                        let out = self( $( $callback, )? $( $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))
+                        world.begin_access_scope()?;
+                        let ret = {
+                            #[allow(unused_mut,unused_variables)]
+                            let mut current_arg = 0;
+                            $(
+                                current_arg += 1;
+                                let $param = <$param>::from_script($param, world.clone())
+                                    .map_err(|e| InteropError::function_arg_conversion_error(current_arg.to_string(), e))?;
+                            )*
+                            let out = self( $( $callback, )? $( $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 have been dropped
+                        unsafe { world.end_access_scope()? };
+                        ret
                     })();
                     let script_value: ScriptValue = res.into();
                     script_value

crates/bevy_mod_scripting_core/src/bindings/world.rs

@@ -289,6 +289,22 @@ impl<'w> WorldAccessGuard<'w> {
         }))
     }
 
+    /// 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> {
+        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()));
+        }
+
+        Ok(())
+    }
+
+    /// 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> {
+        self.0.accesses.release_all_accesses();
+
+        Ok(())
+    }
+
     /// Purely debugging utility to list all accesses currently held.
     pub fn list_accesses(&self) -> Vec<(ReflectAccessId, AccessCount)> {
         self.0.accesses.list_accesses()

crates/bevy_mod_scripting_core/src/error.rs

@@ -392,6 +392,14 @@ impl InteropError {
         }))
     }
 
+    pub fn invalid_access_count(count: usize, expected: usize, context: String) -> Self {
+        Self(Arc::new(InteropErrorInner::InvalidAccessCount {
+            count,
+            expected,
+            context,
+        }))
+    }
+
     pub fn inner(&self) -> &InteropErrorInner {
         &self.0
     }
@@ -429,6 +437,11 @@ pub enum InteropErrorInner {
         base: ReflectBaseType,
         location: Option<std::panic::Location<'static>>,
     },
+    InvalidAccessCount {
+        count: usize,
+        expected: usize,
+        context: String,
+    },
     ImpossibleConversion {
         into: TypeId,
     },
@@ -645,6 +658,9 @@ impl DisplayWithWorld for InteropErrorInner {
             InteropErrorInner::LengthMismatch { expected, got } => {
                 format!("Array/List Length mismatch, expected: {}, got: {}", expected, got)
             },
+            InteropErrorInner::InvalidAccessCount { count, expected, context } => {
+                format!("Invalid access count, expected: {}, got: {}. {}", expected, count, context)
+            },
         }
     }
 }

crates/languages/bevy_mod_scripting_lua/tests/data/access/aliasing_global_access.lua

@@ -0,0 +1,11 @@
+local entity = Entity.from_raw(9999);
+_claim_global_access();
+
+if pcall(function()
+    entity:eq(entity)
+end)
+then
+    error("Aliasing access did not panick")
+else 
+    -- all good
+end

crates/languages/bevy_mod_scripting_lua/tests/data/access/aliasing_write.lua

@@ -0,0 +1,11 @@
+local entity = Entity.from_raw(9999);
+_claim_write_access(entity);
+
+if pcall(function()
+    entity:eq(entity)
+end)
+then
+    error("Aliasing access did not panick")
+else 
+    -- all good
+end

crates/languages/bevy_mod_scripting_lua/tests/data/access/multiple_read_refs.lua

@@ -0,0 +1,3 @@
+local entity = Entity.from_raw(9999);
+-- does not throw
+entity:eq(entity);

crates/languages/bevy_mod_scripting_lua/tests/lua_tests.rs

@@ -7,8 +7,8 @@ use bevy::{
 };
 use bevy_mod_scripting_core::{
     bindings::{
-        pretty_print::DisplayWithWorld, ReflectAllocator, ReflectReference, ScriptTypeRegistration,
-        WorldAccessGuard, WorldCallbackAccess,
+        access_map::ReflectAccessId, pretty_print::DisplayWithWorld, ReflectAllocator,
+        ReflectReference, ScriptTypeRegistration, WorldAccessGuard, WorldCallbackAccess,
     },
     context::ContextLoadingSettings,
     error::ScriptError,
@@ -97,6 +97,16 @@ fn init_lua_test_utils(_script_name: &Cow<'static, str>, lua: &mut Lua) -> Resul
     let assert_throws = lua
         .create_function(|lua, (f, regex): (LuaFunction, String)| {
             let world = lua.get_world();
+
+            // let result = match std::panic::catch_unwind(|| ) {
+            //     Ok(e) => e,
+            //     Err(panic) => {
+            //         return Err(mlua::Error::RuntimeError(format!(
+            //             "Function panicked: {:?}",
+            //             panic
+            //         )))
+            //     }
+            // };
             let result = f.call::<()>(());
             let err = match result {
                 Ok(_) => {
@@ -120,6 +130,34 @@ fn init_lua_test_utils(_script_name: &Cow<'static, str>, lua: &mut Lua) -> Resul
         })
         .unwrap();
 
+    let set_write_access = lua
+        .create_function(|lua, val: LuaReflectReference| {
+            let world = lua.get_world();
+            let inner: ReflectReference = val.into();
+
+            world.claim_write_access(ReflectAccessId::for_reference(inner.base.base_id).unwrap());
+            Ok(())
+        })
+        .unwrap();
+
+    let set_read_access = lua
+        .create_function(|lua, val: LuaReflectReference| {
+            let world = lua.get_world();
+            let inner: ReflectReference = val.into();
+
+            world.claim_read_access(ReflectAccessId::for_reference(inner.base.base_id).unwrap());
+            Ok(())
+        })
+        .unwrap();
+
+    let claim_whole_world_access = lua
+        .create_function(|lua, ()| {
+            let world = lua.get_world();
+            world.claim_global_access();
+            Ok(())
+        })
+        .unwrap();
+
     let globals = lua.globals();
     globals
         .set(
@@ -131,6 +169,14 @@ fn init_lua_test_utils(_script_name: &Cow<'static, str>, lua: &mut Lua) -> Resul
     globals.set("assert_throws", assert_throws).unwrap();
 
     globals.set("_get_mock_type", _get_mock_type).unwrap();
+
+    globals
+        .set("_claim_write_access", set_write_access)
+        .unwrap();
+    globals.set("_claim_read_access", set_read_access).unwrap();
+    globals
+        .set("_claim_global_access", claim_whole_world_access)
+        .unwrap();
     Ok(())
 }
 
@@ -184,12 +230,6 @@ impl Test {
             Failed::from(msg)
         })?;
 
-        // WorldCallbackAccess::with_callback_access(app.world_mut(), |world| {
-        //     lua.globals().set("world", LuaWorld(world.clone())).unwrap();
-
-        //     let code = lua.load(self.code).set_name(self.path.to_string_lossy());
-        //     code.exec().map_err(|e| e.to_string())
-        // })?;
         Ok(())
     }