Skip to content

fix!(core): Make HubSwitchGuard !Send to prevent thread corruption #957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
szokeasaurusrex wants to merge 1 commit into
base: master
Choose a base branch
from
+40 −14
Draft

fix!(core): Make HubSwitchGuard !Send to prevent thread corruption #957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## Unreleased

### Fixes

- Fixed thread corruption bug where `HubSwitchGuard` could be dropped on wrong thread ([#957](https://github.com/getsentry/sentry-rust/pull/957))
- **Breaking change**: `sentry_core::HubSwitchGuard` is now `!Send`, preventing it from being moved across threads. Code that previously sent the guard to another thread will no longer compile.

## 0.46.1

### Improvements
Expand Down
16 changes: 12 additions & 4 deletions sentry-core/src/hub_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::cell::{Cell, UnsafeCell};
use std::sync::{Arc, LazyLock, PoisonError, RwLock};
use std::marker::PhantomData;
use std::sync::{Arc, LazyLock, MutexGuard, PoisonError, RwLock};
use std::thread;

use crate::Scope;
Expand All @@ -19,10 +20,14 @@ thread_local! {
);
}

/// A Hub switch guard used to temporarily swap
/// active hub in thread local storage.
/// A guard that temporarily swaps the active hub in thread-local storage.
///
/// This type is `!Send` because it manages thread-local state and must be
/// dropped on the same thread where it was created.
pub struct SwitchGuard {
inner: Option<(Arc<Hub>, bool)>,
/// Makes this type `!Send` while keeping it `Sync`.
_not_send: PhantomData<MutexGuard<'static, ()>>,
}

impl SwitchGuard {
Expand All @@ -41,7 +46,10 @@ impl SwitchGuard {
let was_process_hub = is_process_hub.replace(false);
Some((hub, was_process_hub))
});
SwitchGuard { inner }
SwitchGuard {
inner,
_not_send: PhantomData,
}
}

fn swap(&mut self) -> Option<Arc<Hub>> {
Expand Down
31 changes: 21 additions & 10 deletions sentry-tracing/src/layer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::borrow::Cow;
use std::cell::RefCell;
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};
use std::sync::Arc;

use bitflags::bitflags;
Expand Down Expand Up @@ -236,7 +236,6 @@ pub(super) struct SentrySpanData {
pub(super) sentry_span: TransactionOrSpan,
parent_sentry_span: Option<TransactionOrSpan>,
hub: Arc<sentry_core::Hub>,
hub_switch_guard: Option<sentry_core::HubSwitchGuard>,
Copy link
Member Author

@szokeasaurusrex szokeasaurusrex Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lcian I've been using Claude to work on this PR. It removed this field to keep SentrySpanData Send even after HubSwitchGuard has been made !Send. However, to me, it does not make that much sense that a span should move between threads, so I am not sure it should be Send. Claude says maybe in async contexts it makes sense.

What do you think?

Copy link
Member

@lcian lcian Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span data needs to be Send due to this trait bound
https://github.com/tokio-rs/tracing/blob/efc690fa6bd1d9c3a57528b9bc8ac80504a7a6ed/tracing-subscriber/src/registry/extensions.rs#L87

Logically, that makes complete sense.
Say I have:

#[tracing::instrument]
async fn f() {
  // some async code
}

Then the behavior I want is for the span tracking the execution of f to be entered and exited every time the future representing f is polled. And if I'm using tokio, in general, the future can be polled on any thread, it can freely move.
Therefore a Span and its extensions need to be Send.

Technically, the extensions are not stored on the Span itself but rather in the Registry, however the intuition above still stands.

I will have a better look at this PR tomorrow.

Copy link
Member

@lcian lcian Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same reason why Hub itself needs to be send by the way.
Anyways I think this PR might be beneficial for the case that the user reported, and I would like a test which we can run with and without the change to see the difference.

It is possible that to do this test you will need a way to distinguish between hubs which right now is not directly possible. For a while I wanted to introduce a UUID on the Hub just to identify which would be helpful for debugging and tests like this, so that should be a way.

Copy link
Member Author

@szokeasaurusrex szokeasaurusrex Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will try this out. It is likely I will only get around to it during the week of January 26, since I have some higher prio stuff the rest of this week and vacation next week

}

impl<S> Layer<S> for SentryLayer<S>
Expand Down Expand Up @@ -338,7 +337,6 @@ where
sentry_span,
parent_sentry_span,
hub,
hub_switch_guard: None,
});
}

Expand All @@ -350,12 +348,15 @@ where
None => return,
};

let mut extensions = span.extensions_mut();
if let Some(data) = extensions.get_mut::<SentrySpanData>() {
data.hub_switch_guard = Some(sentry_core::HubSwitchGuard::new(data.hub.clone()));
let extensions = span.extensions();
if let Some(data) = extensions.get::<SentrySpanData>() {
let guard = sentry_core::HubSwitchGuard::new(data.hub.clone());
SPAN_GUARDS.with(|guards| {
guards.borrow_mut().insert(id.clone(), guard);
});
data.hub.configure_scope(|scope| {
scope.set_span(Some(data.sentry_span.clone()));
})
});
}
}

Expand All @@ -366,18 +367,24 @@ where
None => return,
};

let mut extensions = span.extensions_mut();
if let Some(data) = extensions.get_mut::<SentrySpanData>() {
let extensions = span.extensions();
if let Some(data) = extensions.get::<SentrySpanData>() {
data.hub.configure_scope(|scope| {
scope.set_span(data.parent_sentry_span.clone());
});
data.hub_switch_guard.take();
}
SPAN_GUARDS.with(|guards| {
guards.borrow_mut().remove(id);
});
}

/// When a span gets closed, finish the underlying sentry span, and set back
/// its parent as the *current* sentry span.
fn on_close(&self, id: span::Id, ctx: Context<'_, S>) {
SPAN_GUARDS.with(|guards| {
guards.borrow_mut().remove(&id);
});

let span = match ctx.span(&id) {
Some(span) => span,
None => return,
Expand Down Expand Up @@ -503,6 +510,10 @@ fn extract_span_data(

thread_local! {
static VISITOR_BUFFER: RefCell<String> = const { RefCell::new(String::new()) };
/// Hub switch guards keyed by span ID. Stored in thread-local so guards are
/// always dropped on the same thread where they were created.
static SPAN_GUARDS: RefCell<HashMap<span::Id, sentry_core::HubSwitchGuard>> =
RefCell::new(HashMap::new());
}

/// Records all span fields into a `BTreeMap`, reusing a mutable `String` as buffer.
Expand Down