From 7a99428be29759b53cb43a01f3cbfe0a3e78a649 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Thu, 24 Nov 2022 18:48:32 +0100 Subject: [PATCH 1/2] ref: Switch Hubs using a Guard Use a SwitchGuard internally as a replacement for `Hub::run` which does not require passing a closure, and does not go through `panic::catch_unwind`, as `Drop` impls are guaranteed to be executed even when panics are unwinding. --- sentry-core/src/futures.rs | 3 +- sentry-core/src/hub_impl.rs | 94 +++++++++++++++++++++---------------- 2 files changed, 55 insertions(+), 42 deletions(-) diff --git a/sentry-core/src/futures.rs b/sentry-core/src/futures.rs index dd8d4d71..5d8e7f05 100644 --- a/sentry-core/src/futures.rs +++ b/sentry-core/src/futures.rs @@ -37,7 +37,8 @@ where let future = unsafe { self.map_unchecked_mut(|s| &mut s.future) }; #[cfg(feature = "client")] { - Hub::run(hub, || future.poll(cx)) + let _guard = crate::hub_impl::SwitchGuard::new(hub); + future.poll(cx) } #[cfg(not(feature = "client"))] { diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 7942786d..7300463a 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -20,6 +20,57 @@ thread_local! { static USE_PROCESS_HUB: Cell = Cell::new(PROCESS_HUB.1 == thread::current().id()); } +pub(crate) struct SwitchGuard { + inner: Option<(Arc, bool)>, +} + +impl SwitchGuard { + pub(crate) fn new(hub: Arc) -> Self { + let inner = THREAD_HUB.with(|ctx| unsafe { + let ptr = ctx.get(); + if std::ptr::eq(&**ptr, &*hub) { + None + } else { + let was_process_hub = USE_PROCESS_HUB.with(|x| { + if x.get() { + x.set(false); + true + } else { + false + } + }); + let old = (*ptr).clone(); + *ptr = hub; + Some((old, was_process_hub)) + } + }); + SwitchGuard { inner } + } + + fn switch_internal(&mut self) -> Option> { + if let Some((old_hub, was_process_hub)) = self.inner.take() { + let current_hub = THREAD_HUB.with(|ctx| unsafe { + let ptr = ctx.get(); + let current_hub = (*ptr).clone(); + *ptr = old_hub; + current_hub + }); + if was_process_hub { + USE_PROCESS_HUB.with(|x| x.set(true)); + } + Some(current_hub) + } else { + None + } + } +} + +impl Drop for SwitchGuard { + fn drop(&mut self) { + let _ = self.switch_internal(); + } +} + #[derive(Debug)] pub(crate) struct HubImpl { pub(crate) stack: Arc>, @@ -125,47 +176,8 @@ impl Hub { /// Once the function is finished executing, including after it /// paniced, the original hub is re-installed if one was present. pub fn run R, R>(hub: Arc, f: F) -> R { - let mut restore_process_hub = false; - let did_switch = THREAD_HUB.with(|ctx| unsafe { - let ptr = ctx.get(); - if std::ptr::eq(&**ptr, &*hub) { - None - } else { - USE_PROCESS_HUB.with(|x| { - if x.get() { - restore_process_hub = true; - x.set(false); - } - }); - let old = (*ptr).clone(); - *ptr = hub.clone(); - Some(old) - } - }); - - match did_switch { - None => { - // None means no switch happened. We can invoke the function - // just like that, no changes necessary. - f() - } - Some(old_hub) => { - use std::panic; - - // this is for the case where we just switched the hub. This - // means we need to catch the panic, restore the - // old context and resume the panic if needed. - let rv = panic::catch_unwind(panic::AssertUnwindSafe(f)); - THREAD_HUB.with(|ctx| unsafe { *ctx.get() = old_hub }); - if restore_process_hub { - USE_PROCESS_HUB.with(|x| x.set(true)); - } - match rv { - Err(err) => panic::resume_unwind(err), - Ok(rv) => rv, - } - } - } + let _guard = SwitchGuard::new(hub); + f() } /// Returns the currently bound client. From 88a30a751710d04e59343c0987e65d0fb7a4da91 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Thu, 24 Nov 2022 19:20:22 +0100 Subject: [PATCH 2/2] ref: Use arc-swap --- sentry-core/Cargo.toml | 7 ++-- sentry-core/src/hub_impl.rs | 74 ++++++++++++++++--------------------- 2 files changed, 35 insertions(+), 46 deletions(-) diff --git a/sentry-core/Cargo.toml b/sentry-core/Cargo.toml index 5f3f910d..26ca759b 100644 --- a/sentry-core/Cargo.toml +++ b/sentry-core/Cargo.toml @@ -21,7 +21,7 @@ harness = false [features] default = [] -client = ["rand"] +client = ["rand", "arc-swap"] # I would love to just have a `log` feature, but this is used inside a macro, # and macros actually expand features (and extern crate) where they are used! debug-logs = ["dep:log"] @@ -41,10 +41,11 @@ sys-info = { version = "0.9.1", optional = true } build_id = { version = "0.2.1", optional = true } findshlibs = { version = "=0.10.2", optional = true } rustc_version_runtime = { version = "0.2.1", optional = true } -indexmap = { version = "1.9.1", optional = true} +indexmap = { version = "1.9.1", optional = true } +arc-swap = { version = "1.5.1", optional = true } [target.'cfg(target_family = "unix")'.dependencies] -pprof = { version = "0.11.0", optional = true, default-features = false} +pprof = { version = "0.11.0", optional = true, default-features = false } libc = { version = "^0.2.66", optional = true } [dev-dependencies] diff --git a/sentry-core/src/hub_impl.rs b/sentry-core/src/hub_impl.rs index 7300463a..c3e5db11 100644 --- a/sentry-core/src/hub_impl.rs +++ b/sentry-core/src/hub_impl.rs @@ -1,10 +1,11 @@ -use std::cell::{Cell, UnsafeCell}; +use std::cell::Cell; use std::sync::{Arc, PoisonError, RwLock}; use std::thread; use crate::Scope; use crate::{scope::Stack, Client, Hub}; +use arc_swap::ArcSwap; use once_cell::sync::Lazy; static PROCESS_HUB: Lazy<(Arc, thread::ThreadId)> = Lazy::new(|| { @@ -15,9 +16,10 @@ static PROCESS_HUB: Lazy<(Arc, thread::ThreadId)> = Lazy::new(|| { }); thread_local! { - static THREAD_HUB: UnsafeCell> = UnsafeCell::new( - Arc::new(Hub::new_from_top(&PROCESS_HUB.0))); - static USE_PROCESS_HUB: Cell = Cell::new(PROCESS_HUB.1 == thread::current().id()); + static THREAD_HUB: (ArcSwap, Cell) = ( + ArcSwap::from_pointee(Hub::new_from_top(&PROCESS_HUB.0)), + Cell::new(PROCESS_HUB.1 == thread::current().id()) + ); } pub(crate) struct SwitchGuard { @@ -26,39 +28,29 @@ pub(crate) struct SwitchGuard { impl SwitchGuard { pub(crate) fn new(hub: Arc) -> Self { - let inner = THREAD_HUB.with(|ctx| unsafe { - let ptr = ctx.get(); - if std::ptr::eq(&**ptr, &*hub) { - None - } else { - let was_process_hub = USE_PROCESS_HUB.with(|x| { - if x.get() { - x.set(false); - true - } else { - false - } - }); - let old = (*ptr).clone(); - *ptr = hub; - Some((old, was_process_hub)) + let inner = THREAD_HUB.with(|(old_hub, is_process_hub)| { + { + let old_hub = old_hub.load(); + if std::ptr::eq(old_hub.as_ref(), hub.as_ref()) { + return None; + } } + let old_hub = old_hub.swap(hub); + let was_process_hub = is_process_hub.replace(false); + Some((old_hub, was_process_hub)) }); SwitchGuard { inner } } - fn switch_internal(&mut self) -> Option> { + fn swap(&mut self) -> Option> { if let Some((old_hub, was_process_hub)) = self.inner.take() { - let current_hub = THREAD_HUB.with(|ctx| unsafe { - let ptr = ctx.get(); - let current_hub = (*ptr).clone(); - *ptr = old_hub; - current_hub - }); - if was_process_hub { - USE_PROCESS_HUB.with(|x| x.set(true)); - } - Some(current_hub) + Some(THREAD_HUB.with(|(hub, is_process_hub)| { + let hub = hub.swap(old_hub); + if was_process_hub { + is_process_hub.set(true); + } + hub + })) } else { None } @@ -67,7 +59,7 @@ impl SwitchGuard { impl Drop for SwitchGuard { fn drop(&mut self) { - let _ = self.switch_internal(); + let _ = self.swap(); } } @@ -154,17 +146,13 @@ impl Hub { where F: FnOnce(&Arc) -> R, { - if USE_PROCESS_HUB.with(Cell::get) { - f(&PROCESS_HUB.0) - } else { - // note on safety: this is safe because even though we change the Arc - // by temporary binding we guarantee that the original Arc stays alive. - // For more information see: run - THREAD_HUB.with(|stack| unsafe { - let ptr = stack.get(); - f(&*ptr) - }) - } + THREAD_HUB.with(|(hub, is_process_hub)| { + if is_process_hub.get() { + f(&PROCESS_HUB.0) + } else { + f(&hub.load()) + } + }) } /// Binds a hub to the current thread for the duration of the call.