Skip to content

Commit

Permalink
reflect: avoid deadlock in GenericTypeCell (#8957)
Browse files Browse the repository at this point in the history
# Objective

- There was a deadlock discovered in the implementation of
`bevy_reflect::utility::GenericTypeCell`, when called on a recursive
type, e.g. `Vec<Vec<VariableCurve>>`

## Solution

- Drop the lock before calling the initialisation function, and then
pick it up again afterwards.

## Additional Context
- [Discussed on
Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1122706835284185108)
  • Loading branch information
soqb authored Jun 27, 2023
1 parent 10f5c92 commit e17fc53
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 14 deletions.
9 changes: 9 additions & 0 deletions crates/bevy_reflect/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1848,6 +1848,15 @@ bevy_reflect::tests::should_reflect_debug::Test {
assert_eq!("123", format!("{:?}", foo));
}

#[test]
fn recursive_typed_storage_does_not_hang() {
#[derive(Reflect)]
struct Recurse<T>(T);

let _ = <Recurse<Recurse<()>> as Typed>::type_info();
let _ = <Recurse<Recurse<()>> as TypePath>::type_path();
}

#[cfg(feature = "glam")]
mod glam {
use super::*;
Expand Down
41 changes: 27 additions & 14 deletions crates/bevy_reflect/src/utility.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Helpers for working with Bevy reflection.
use crate::TypeInfo;
use bevy_utils::{FixedState, HashMap};
use bevy_utils::{FixedState, StableHashMap};
use once_cell::race::OnceBox;
use parking_lot::RwLock;
use std::{
Expand Down Expand Up @@ -206,7 +206,7 @@ impl<T: TypedProperty> NonGenericTypeCell<T> {
/// ```
/// [`impl_type_path`]: crate::impl_type_path
/// [`TypePath`]: crate::TypePath
pub struct GenericTypeCell<T: TypedProperty>(OnceBox<RwLock<HashMap<TypeId, &'static T::Stored>>>);
pub struct GenericTypeCell<T: TypedProperty>(RwLock<StableHashMap<TypeId, &'static T::Stored>>);

/// See [`GenericTypeCell`].
pub type GenericTypeInfoCell = GenericTypeCell<TypeInfo>;
Expand All @@ -216,7 +216,9 @@ pub type GenericTypePathCell = GenericTypeCell<TypePathComponent>;
impl<T: TypedProperty> GenericTypeCell<T> {
/// Initialize a [`GenericTypeCell`] for generic types.
pub const fn new() -> Self {
Self(OnceBox::new())
// Use `bevy_utils::StableHashMap` over `bevy_utils::HashMap`
// because `BuildHasherDefault` is unfortunately not const.
Self(RwLock::new(StableHashMap::with_hasher(FixedState)))
}

/// Returns a reference to the [`TypedProperty`] stored in the cell.
Expand All @@ -229,19 +231,30 @@ impl<T: TypedProperty> GenericTypeCell<T> {
F: FnOnce() -> T::Stored,
{
let type_id = TypeId::of::<G>();
// let mapping = self.0.get_or_init(|| Box::new(RwLock::default()));
let mapping = self.0.get_or_init(Box::default);
if let Some(info) = mapping.read().get(&type_id) {
return info;

// Put in a seperate scope, so `mapping` is dropped before `f`,
// since `f` might want to call `get_or_insert` recursively
// and we don't want a deadlock!
{
let mapping = self.0.read();
if let Some(info) = mapping.get(&type_id) {
return info;
}
}

mapping.write().entry(type_id).or_insert_with(|| {
// We leak here in order to obtain a `&'static` reference.
// Otherwise, we won't be able to return a reference due to the `RwLock`.
// This should be okay, though, since we expect it to remain statically
// available over the course of the application.
Box::leak(Box::new(f()))
})
let value = f();

let mut mapping = self.0.write();
mapping
.entry(type_id)
.insert({
// We leak here in order to obtain a `&'static` reference.
// Otherwise, we won't be able to return a reference due to the `RwLock`.
// This should be okay, though, since we expect it to remain statically
// available over the course of the application.
Box::leak(Box::new(value))
})
.get()
}
}

Expand Down

0 comments on commit e17fc53

Please sign in to comment.