fix!(core): Make HubSwitchGuard !Send to prevent thread corruption #957
Conversation
szokeasaurusrex
force-pushed
the
szokeasaurusrex/hubswitchguard
branch
from
21b407b to
1dea844
Compare
szokeasaurusrex
changed the title
HubSwitchGuard manages thread-local hub state but was Send, allowing it to be moved to another thread. When dropped on the wrong thread, it would corrupt that thread's hub state instead of restoring the original thread. To fix this, add PhantomData<MutexGuard<'static, ()>> to make the guard !Send while keeping it Sync. This prevents the guard from being moved across threads at compile time. Additionally, refactor sentry-tracing to store guards in thread-local storage keyed by span ID instead of in span extensions. This fixes a related bug where multiple threads entering the same span would clobber each other's guards. Fixes #943 Fixes #946 Refs RUST-130 Refs RUST-132 Co-Authored-By: Claude <noreply@anthropic.com>
szokeasaurusrex
force-pushed
the
szokeasaurusrex/hubswitchguard
branch
from
e78483a to
8abf004
Compare
| pub(super) sentry_span: TransactionOrSpan, | ||
| parent_sentry_span: Option<TransactionOrSpan>, | ||
| hub: Arc<sentry_core::Hub>, | ||
| hub_switch_guard: Option<sentry_core::HubSwitchGuard>, |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Description
HubSwitchGuardmanages thread-local hub state but wasSend, allowing it to be moved to another thread. When dropped on the wrong thread, it would corrupt that thread's hub state instead of restoring the original thread.This PR makes
HubSwitchGuard!Sendby addingPhantomData<MutexGuard<'static, ()>>while keeping itSync. The type system now prevents the guard from being moved across threads at compile time.Additionally, this refactors
sentry-tracingto store guards in thread-local storage keyed by span ID instead of in span extensions. This fixes a related bug where multiple threads entering the same span would clobber each other's guards because they shared the same span extension storage.Issues