search

getsentry / sentry-rust

Official Sentry SDK for Rust
https://sentry.io/
Apache License 2.0
620 stars 153 forks source link

Threads mistakenly attach to main thread's span #674

Open Ten0 opened 4 months ago

Ten0 commented 4 months ago

The first time we call sentry-related functions in a thread, it attaches to whichever Span the main thread is currently attached to. This causes very wrong sub-spanning (which I just spend 6 hours investigating). Attaching to sentry client, DSN, parameters..., etc seems correct, but span definitely not.

I feel like maybe the Scope should be a property that's not inherited from whichever scope the main thread happens to be in at that moment, because if there is a sub-thread it's very possible that the main thread is doing unrelated work at that moment, and that unrelated work may very well involve its own spans and data, so it seems that it's not correct to attach to those.

Related issues: #507 Related writeup: https://swatinem.de/blog/log-contexts/

Ten0 commented 4 months ago

Aha digging further into this it seems that it already attaches to the top scope of the main thread, so the issue may be that the sentry initialization leaves without actually pushing a scope level, leading to it accidentally polluting what others need to attach to...

Ten0 commented 4 months ago

so the issue may be that the sentry initialization leaves without actually pushing a scope level

That actually wouldn't work: any Hub::new_from_top(Hub::current()) on the main thead itself wouldn't attach to the proper thing.

Ten0 commented 4 months ago

Ok so I'm trying to understand how it's supposed to work but there's something I don't understand: Hub::push_scope() ends up calling this function, but: https://github.com/getsentry/sentry-rust/blob/8cc4848f115152af6f2ad66d7aeb17bd3bbe38fd/sentry-core/src/scope/real.rs#L86-L89 actually pushes a clone of StackLayer which contains an Arc<Scope>: https://github.com/getsentry/sentry-rust/blob/8cc4848f115152af6f2ad66d7aeb17bd3bbe38fd/sentry-core/src/scope/real.rs#L72-L76 without actually touching the top, so in effect we aren't changing the scope at all, since the top of the stack (and even the new layer) end up referencing the same scope as before! So, what's the point of this push_scope function, since it actually can't change the active scope?

EDIT: Aah the update actually happens here, and all of the data of the scope is cloned and they all have Arc wrappers: https://github.com/getsentry/sentry-rust/blob/8cc4848f115152af6f2ad66d7aeb17bd3bbe38fd/sentry-core/src/hub.rs#L210-L213

Ten0 commented 4 months ago

Ok so my understanding is that there's no way to prevent the Hub to attach to the current scope of the main thread. It looks like the "main Hub" shouldn't really be attached to any thread, and even the main thread should have its own Hub. That would fix this issue.