Skip to content

Commit e17fc53

Browse files
authored
reflect: avoid deadlock in GenericTypeCell (#8957)
# 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)
1 parent 10f5c92 commit e17fc53

File tree

2 files changed

+36
-14
lines changed

2 files changed

+36
-14
lines changed

crates/bevy_reflect/src/lib.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1848,6 +1848,15 @@ bevy_reflect::tests::should_reflect_debug::Test {
18481848
assert_eq!("123", format!("{:?}", foo));
18491849
}
18501850

1851+
#[test]
1852+
fn recursive_typed_storage_does_not_hang() {
1853+
#[derive(Reflect)]
1854+
struct Recurse<T>(T);
1855+
1856+
let _ = <Recurse<Recurse<()>> as Typed>::type_info();
1857+
let _ = <Recurse<Recurse<()>> as TypePath>::type_path();
1858+
}
1859+
18511860
#[cfg(feature = "glam")]
18521861
mod glam {
18531862
use super::*;

crates/bevy_reflect/src/utility.rs

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
//! Helpers for working with Bevy reflection.
22
33
use crate::TypeInfo;
4-
use bevy_utils::{FixedState, HashMap};
4+
use bevy_utils::{FixedState, StableHashMap};
55
use once_cell::race::OnceBox;
66
use parking_lot::RwLock;
77
use std::{
@@ -206,7 +206,7 @@ impl<T: TypedProperty> NonGenericTypeCell<T> {
206206
/// ```
207207
/// [`impl_type_path`]: crate::impl_type_path
208208
/// [`TypePath`]: crate::TypePath
209-
pub struct GenericTypeCell<T: TypedProperty>(OnceBox<RwLock<HashMap<TypeId, &'static T::Stored>>>);
209+
pub struct GenericTypeCell<T: TypedProperty>(RwLock<StableHashMap<TypeId, &'static T::Stored>>);
210210

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

222224
/// Returns a reference to the [`TypedProperty`] stored in the cell.
@@ -229,19 +231,30 @@ impl<T: TypedProperty> GenericTypeCell<T> {
229231
F: FnOnce() -> T::Stored,
230232
{
231233
let type_id = TypeId::of::<G>();
232-
// let mapping = self.0.get_or_init(|| Box::new(RwLock::default()));
233-
let mapping = self.0.get_or_init(Box::default);
234-
if let Some(info) = mapping.read().get(&type_id) {
235-
return info;
234+
235+
// Put in a seperate scope, so `mapping` is dropped before `f`,
236+
// since `f` might want to call `get_or_insert` recursively
237+
// and we don't want a deadlock!
238+
{
239+
let mapping = self.0.read();
240+
if let Some(info) = mapping.get(&type_id) {
241+
return info;
242+
}
236243
}
237244

238-
mapping.write().entry(type_id).or_insert_with(|| {
239-
// We leak here in order to obtain a `&'static` reference.
240-
// Otherwise, we won't be able to return a reference due to the `RwLock`.
241-
// This should be okay, though, since we expect it to remain statically
242-
// available over the course of the application.
243-
Box::leak(Box::new(f()))
244-
})
245+
let value = f();
246+
247+
let mut mapping = self.0.write();
248+
mapping
249+
.entry(type_id)
250+
.insert({
251+
// We leak here in order to obtain a `&'static` reference.
252+
// Otherwise, we won't be able to return a reference due to the `RwLock`.
253+
// This should be okay, though, since we expect it to remain statically
254+
// available over the course of the application.
255+
Box::leak(Box::new(value))
256+
})
257+
.get()
245258
}
246259
}
247260

0 commit comments

Comments
 (0)