From a98c2daed34fac8158691fa3b78a0f71ba4e5336 Mon Sep 17 00:00:00 2001 From: Benjamin Saunders Date: Fri, 26 Nov 2021 13:55:46 -0800 Subject: [PATCH] Leave World in a consistent state after query borrowck panics Previously, some archetypes might be left permanently borrowed, making the World unusable. --- CHANGELOG.md | 4 +++ src/archetype.rs | 44 +++++++++++++++++++++++++++++---- src/lib.rs | 2 +- src/query.rs | 64 +++++++++++++++++++++++++++++++++--------------- src/query_one.rs | 4 ++- tests/tests.rs | 19 ++++++++++++++ 6 files changed, 110 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0aa0cf13..63537184 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ older versions may fail. - `ColumnRef` renamed to `ArchetypeColumn` +### Fixed +- If a query panics due to a dynamic borrow-checking failure, any borrows that query already + acquired are released. + # 0.6.5 ### Changed diff --git a/src/archetype.rs b/src/archetype.rs index 6e5c39b4..3e52f2c9 100644 --- a/src/archetype.rs +++ b/src/archetype.rs @@ -127,18 +127,34 @@ impl Archetype { } pub(crate) fn borrow(&self, state: usize) { + if let Err(e) = self.try_borrow::(state) { + panic!("{}", e); + } + } + + pub(crate) fn borrow_mut(&self, state: usize) { + if let Err(e) = self.try_borrow_mut::(state) { + panic!("{}", e); + } + } + + pub(crate) fn try_borrow(&self, state: usize) -> Result<(), BorrowError> { assert_eq!(self.types[state].id, TypeId::of::()); - if !self.data[state].state.borrow() { - panic!("{} already borrowed uniquely", type_name::()); + if self.data[state].state.borrow() { + Ok(()) + } else { + Err(BorrowError::Shared(type_name::())) } } - pub(crate) fn borrow_mut(&self, state: usize) { + pub(crate) fn try_borrow_mut(&self, state: usize) -> Result<(), BorrowError> { assert_eq!(self.types[state].id, TypeId::of::()); - if !self.data[state].state.borrow_mut() { - panic!("{} already borrowed", type_name::()); + if self.data[state].state.borrow_mut() { + Ok(()) + } else { + Err(BorrowError::Unique(type_name::())) } } @@ -626,3 +642,21 @@ impl fmt::Debug for ArchetypeColumn<'_, T> { self.column.fmt(f) } } + +#[doc(hidden)] +pub enum BorrowError { + /// Error borrowing &T + Shared(&'static str), + /// Error borrowing &mut T + Unique(&'static str), +} + +impl fmt::Display for BorrowError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use BorrowError::*; + match *self { + Shared(ty) => write!(f, "{} already borrowed uniquely", ty), + Unique(ty) => write!(f, "{} already borrowed", ty), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index ba014ad9..81c06917 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -91,7 +91,7 @@ pub use world::{ // Unstable implementation details needed by the macros #[doc(hidden)] -pub use archetype::TypeInfo; +pub use archetype::{BorrowError, TypeInfo}; #[cfg(feature = "macros")] #[doc(hidden)] pub use lazy_static; diff --git a/src/query.rs b/src/query.rs index a601adee..9058b133 100644 --- a/src/query.rs +++ b/src/query.rs @@ -14,7 +14,7 @@ use core::slice::Iter as SliceIter; use crate::alloc::boxed::Box; use crate::archetype::Archetype; use crate::entities::EntityMeta; -use crate::{Component, Entity, World}; +use crate::{BorrowError, Component, Entity, World}; /// A collection of component types to fetch from a [`World`](crate::World) pub trait Query { @@ -43,7 +43,7 @@ pub unsafe trait Fetch<'a>: Sized { fn access(archetype: &Archetype) -> Option; /// Acquire dynamic borrows from `archetype` - fn borrow(archetype: &Archetype, state: Self::State); + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError>; /// Look up state for `archetype` if it should be traversed fn prepare(archetype: &Archetype) -> Option; /// Construct a `Fetch` for `archetype` based on the associated state @@ -99,8 +99,8 @@ unsafe impl<'a, T: Component> Fetch<'a> for FetchRead { } } - fn borrow(archetype: &Archetype, state: Self::State) { - archetype.borrow::(state); + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { + archetype.try_borrow::(state) } fn prepare(archetype: &Archetype) -> Option { archetype.get_state::() @@ -145,8 +145,8 @@ unsafe impl<'a, T: Component> Fetch<'a> for FetchWrite { } } - fn borrow(archetype: &Archetype, state: Self::State) { - archetype.borrow_mut::(state); + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { + archetype.try_borrow_mut::(state) } #[allow(clippy::needless_question_mark)] fn prepare(archetype: &Archetype) -> Option { @@ -188,10 +188,11 @@ unsafe impl<'a, T: Fetch<'a>> Fetch<'a> for TryFetch { Some(T::access(archetype).unwrap_or(Access::Iterate)) } - fn borrow(archetype: &Archetype, state: Self::State) { + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { if let Some(state) = state { - T::borrow(archetype, state); + T::borrow(archetype, state)?; } + Ok(()) } fn prepare(archetype: &Archetype) -> Option { Some(T::prepare(archetype)) @@ -326,8 +327,15 @@ unsafe impl<'a, L: Fetch<'a>, R: Fetch<'a>> Fetch<'a> for FetchOr { L::access(archetype).max(R::access(archetype)) } - fn borrow(archetype: &Archetype, state: Self::State) { - state.map(|l| L::borrow(archetype, l), |r| R::borrow(archetype, r)); + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { + let (l, r) = state.split(); + if let Some(l) = l { + L::borrow(archetype, l)?; + } + if let Some(r) = r { + R::borrow(archetype, r)?; + } + Ok(()) } fn prepare(archetype: &Archetype) -> Option { @@ -395,7 +403,7 @@ unsafe impl<'a, T: Component, F: Fetch<'a>> Fetch<'a> for FetchWithout { } } - fn borrow(archetype: &Archetype, state: Self::State) { + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { F::borrow(archetype, state) } fn prepare(archetype: &Archetype) -> Option { @@ -465,7 +473,7 @@ unsafe impl<'a, T: Component, F: Fetch<'a>> Fetch<'a> for FetchWith { } } - fn borrow(archetype: &Archetype, state: Self::State) { + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { F::borrow(archetype, state) } fn prepare(archetype: &Archetype) -> Option { @@ -532,7 +540,9 @@ unsafe impl<'a, F: Fetch<'a>> Fetch<'a> for FetchSatisfies { F::access(archetype).map(|_| Access::Iterate) } - fn borrow(_archetype: &Archetype, _state: Self::State) {} + fn borrow(_archetype: &Archetype, _state: Self::State) -> Result<(), BorrowError> { + Ok(()) + } fn prepare(archetype: &Archetype) -> Option { Some(F::prepare(archetype).is_some()) } @@ -588,10 +598,17 @@ impl<'w, Q: Query> QueryBorrow<'w, Q> { if self.borrowed { return; } - for x in self.archetypes { - // TODO: Release prior borrows on failure? + for (i, x) in self.archetypes.iter().enumerate() { if let Some(state) = Q::Fetch::prepare(x) { - Q::Fetch::borrow(x, state); + if let Err(e) = Q::Fetch::borrow(x, state) { + // Release the borrows we acquired so the `World` remains in a consistent state + for x in &self.archetypes[..i] { + if let Some(state) = Q::Fetch::prepare(x) { + Q::Fetch::release(x, state) + } + } + panic!("{}", e); + } } } self.borrowed = true; @@ -969,9 +986,10 @@ macro_rules! tuple_impl { } #[allow(unused_variables, non_snake_case, clippy::unused_unit)] - fn borrow(archetype: &Archetype, state: Self::State) { + fn borrow(archetype: &Archetype, state: Self::State) -> Result<(), BorrowError> { let ($($name,)*) = state; - $($name::borrow(archetype, $name);)* + $($name::borrow(archetype, $name)?;)* + Ok(()) } #[allow(unused_variables)] fn prepare(archetype: &Archetype) -> Option { @@ -1093,8 +1111,14 @@ impl<'q, Q: Query> PreparedQueryBorrow<'q, Q> { archetypes: &'q [Archetype], state: &'q [(usize, >::State)], ) -> Self { - for (idx, state) in state { - Q::Fetch::borrow(&archetypes[*idx], *state); + for (i, &(idx, archetype_state)) in state.iter().enumerate() { + if let Err(e) = Q::Fetch::borrow(&archetypes[idx], archetype_state) { + // Release the borrows we acquired so the `World` remains in a consistent state + for &(idx, archetype_state) in &state[..i] { + Q::Fetch::release(&archetypes[idx], archetype_state) + } + panic!("{}", e); + } } Self { diff --git a/src/query_one.rs b/src/query_one.rs index 44e15993..8c2ca2af 100644 --- a/src/query_one.rs +++ b/src/query_one.rs @@ -39,7 +39,9 @@ impl<'a, Q: Query> QueryOne<'a, Q> { } unsafe { let state = Q::Fetch::prepare(self.archetype)?; - Q::Fetch::borrow(self.archetype, state); + if let Err(e) = Q::Fetch::borrow(self.archetype, state) { + panic!("{}", e); + } let fetch = Q::Fetch::execute(self.archetype, state); self.borrowed = true; Some(fetch.get(self.index as usize)) diff --git a/tests/tests.rs b/tests/tests.rs index b533b06b..ea689250 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -629,3 +629,22 @@ fn column_get_mut() { } assert_eq!(*world.get::(ent).unwrap(), 99); } + +#[cfg(feature = "std")] +#[test] +fn consistent_borrows_after_panic() { + use std::panic; + + let mut world = World::new(); + world.spawn(("abc", 123)); + let mut q = world.query::<&mut i32>(); + let q_iter = q.iter(); + std::dbg!(); + let result = panic::catch_unwind(|| { + let mut q = world.query::<(&&str, &i32)>(); + let _ = q.iter(); + }); + assert!(result.is_err()); + let mut q = world.query::<&mut &str>(); + let _ = q.iter(); +}