diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 146caa87..299c4ecf 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -1,22 +1,21 @@ -use std::cell::{Cell, UnsafeCell}; +use std::cell::{Cell, RefCell}; use std::marker::PhantomData; use std::sync::{Arc, LazyLock, MutexGuard, PoisonError, RwLock}; use std::thread; +use std::thread::ThreadId; use crate::Scope; use crate::{scope::Stack, Client, Hub}; -static PROCESS_HUB: LazyLock<(Arc, thread::ThreadId)> = LazyLock::new(|| { - ( - Arc::new(Hub::new(None, Arc::new(Default::default()))), - thread::current().id(), - ) +static PROCESS_HUB: LazyLock = LazyLock::new(|| ProcessHub { + hub: Arc::new(Hub::new(None, Arc::new(Default::default()))), + thread: thread::current().id(), }); thread_local! { - static THREAD_HUB: (UnsafeCell>, Cell) = ( - UnsafeCell::new(Arc::new(Hub::new_from_top(&PROCESS_HUB.0))), - Cell::new(PROCESS_HUB.1 == thread::current().id()) + static THREAD_HUB: (RefCell>, Cell) = ( + RefCell::new(Arc::new(Hub::new_from_top(&PROCESS_HUB.hub))), + Cell::new(PROCESS_HUB.thread == thread::current().id()) ); } @@ -50,13 +49,11 @@ impl SwitchGuard { /// to the previous one. pub fn new(mut hub: Arc) -> Self { let inner = THREAD_HUB.with(|(thread_hub, is_process_hub)| { - // SAFETY: `thread_hub` will always be a valid thread local hub, - // by definition not shared between threads. - let thread_hub = unsafe { &mut *thread_hub.get() }; + let mut thread_hub = thread_hub.borrow_mut(); if std::ptr::eq(thread_hub.as_ref(), hub.as_ref()) { return None; } - std::mem::swap(thread_hub, &mut hub); + std::mem::swap(&mut *thread_hub, &mut hub); let was_process_hub = is_process_hub.replace(false); Some((hub, was_process_hub)) }); @@ -69,8 +66,8 @@ impl SwitchGuard { fn swap(&mut self) -> Option> { if let Some((mut hub, was_process_hub)) = self.inner.take() { Some(THREAD_HUB.with(|(thread_hub, is_process_hub)| { - let thread_hub = unsafe { &mut *thread_hub.get() }; - std::mem::swap(thread_hub, &mut hub); + let mut thread_hub = thread_hub.borrow_mut(); + std::mem::swap(&mut *thread_hub, &mut hub); if was_process_hub { is_process_hub.set(true); } @@ -156,7 +153,7 @@ impl Hub { /// This is similar to [`Hub::current`] but instead of picking the /// current thread's hub it returns the main thread's hub instead. pub fn main() -> Arc { - PROCESS_HUB.0.clone() + PROCESS_HUB.hub.clone() } /// Invokes the callback with the default hub. @@ -169,9 +166,13 @@ impl Hub { { THREAD_HUB.with(|(hub, is_process_hub)| { if is_process_hub.get() { - f(&PROCESS_HUB.0) + f(&PROCESS_HUB.hub) } else { - f(unsafe { &*hub.get() }) + // Bind `hub` as `hub.borrow().clone()`. + // It is essential we drop `hub.borrow()` before the callback, otherwise we will + // panic if the callback calls `Hub::run`, so we need the binding. + let hub = hub.borrow().clone(); + f(&hub) } }) } @@ -214,3 +215,29 @@ impl Hub { .with_mut(|stack| f(Arc::make_mut(&mut stack.top_mut().scope))) } } + +/// Helper struct for storing the [`PROCESS_HUB`]. +struct ProcessHub { + /// The process's main hub. + hub: Arc, + /// The thread on which the main hub was initialized. + thread: ThreadId, +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Regression test for [`Hub::with`], ensuring that the `RefCell` borrow is not held during the callback. + /// + /// If we hold the `RefCell` borrow during the callback, this would panic. + #[test] + fn hub_run_inside_with_scope() { + let outer_hub = Arc::new(Hub::new(None, Default::default())); + let inner_hub = Arc::new(Hub::new(None, Default::default())); + + Hub::run(outer_hub, || { + Hub::with(|_| Hub::run(inner_hub, || {})); + }); + } +}