Repository navigation
feat: minidump scope - #1340
feat: minidump scope#1340timfish wants to merge 10 commits into
Conversation
…t hub Behind the thread-registry feature. Each thread registers a slot on first use of its hub, SwitchGuard keeps the slot pointing at the hub current on that thread, and the slot is removed when the thread exits. Hub::for_os_thread looks a hub up by the id current_os_thread_id returns, which is the id a crash handler sees for the crashing thread.
Nothing is sent to the crash reporter until the crash. The on_crash hook in minidumper-child names the crashing OS thread; a parked helper thread serializes the scope of the hub current on that thread (inline on macOS, where other threads are suspended) and the handler sends it before the dump request. The reporter merges it with the fatal event. This replaces the manual set_user/set_tag/set_extra/add_breadcrumb mirror, which cost one message per scope write. Depends on the on_crash hook from the feat/on-crash-hook branch of minidumper-child via a crates-io patch until it is released.
|
👋 Thanks for sending this our way! Before a maintainer reviews the code, we ask community contributors to align with us on the approach first — it keeps your time pointed at changes we can land. The easiest way is to open or find a GitHub issue and discuss the approach with a maintainer there, then link that issue from this PR. If the issue is already assigned to someone else, please check in with them (or with us) before continuing — otherwise two people may end up working on the same task. See our contributing guidelines for the full picture. |
This comment was marked as outdated.
This comment was marked as outdated.
0.6 has the on_crash hook, so the git patch is no longer needed.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
On macOS the crash handler serialized the scope inline, with no timeout. A crash while the crashing thread held the hub lock, the registry mutex or the allocator lock blocked the handler forever, so no minidump was written. The helper thread now runs on every platform. On macOS the handler resumes it with thread_resume, because crash-handler suspends every other thread before the callback. The handler then waits at most scope_timeout, as on Linux and Windows, and requests the dump without scope if the helper is blocked.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit db9875b. Configure here.
It copies the hub's current scope under the read lock only. configure_scope also writes the scope back under the write lock, so a reader that only needs a copy can block on another thread's read lock.
The helper now copies the scope with Hub::scope_snapshot. With configure_scope it took the hub's write lock, and blocked until scope_timeout if the crashing thread held the read lock. On macOS, start now waits until the helper has stored its Mach port. A crash right after init could otherwise find no port, leave the helper suspended and send the event without scope.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Thanks for this!
I found a few potential issues that seem concerning, especially the potential deadlock while crashing.
Besides that, since the change is breaking, I updated the PR title to feat!. I would also like to wait to merge this until we plan to make a breaking release (likely within a few months). However, if it would be possible to make this change backwards compatible, and defer the breaking changes to a later release, that would be even better.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1340 +/- ##
==========================================
+ Coverage 73.81% 74.13% +0.32%
==========================================
Files 64 81 +17
Lines 7538 10278 +2740
==========================================
+ Hits 5564 7620 +2056
- Misses 1974 2658 +684 |
current_os_thread_id now returns Option<u64> and exists only on Linux, Android, macOS and Windows. The pthread_self fallback also compiled on non-unix targets, where libc is not a dependency. On Linux a seccomp filter can make gettid return -1 without running it. Cast to u64, every such thread registered under u64::MAX and overwrote the others. A failed gettid now gives None, and a thread without an id is not registered.
|
I'll look into make this non-breaking! |
set_user, set_tag, set_extra and add_breadcrumb on MinidumpIntegration work as in 0.49.3 again, so this change is not breaking. Each call still sends a scope update to the reporter, which applies it to its hub. The crash event already passes through that hub, so these values merge with the crashing thread's scope.
|
This is now non-breaking and the manual scope sync methods are marked as deprecated. |
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
I found a few more things.
Most important would be to reduce the chunk size, reduce the additional public api surface (also here), and possibly to benchmark whether tracking the hub adds significant overhead, so we can reduce or document this overhead.
| /// One thread's entry in the registry. | ||
| struct Slot { | ||
| /// `None` when the thread has no OS id and is not in the registry. | ||
| os_thread_id: Option<u64>, |
There was a problem hiding this comment.
m: We should not allocate a slot when there is no os_thread_id.
| os_thread_id: Option<u64>, | |
| os_thread_id: u64, |
| fn register(os_thread_id: Option<u64>) -> Self { | ||
| let slot = Arc::new(Slot { | ||
| os_thread_id, | ||
| hub: RwLock::new(Weak::new()), | ||
| }); | ||
| if let Some(id) = os_thread_id { | ||
| REGISTRY | ||
| .lock() | ||
| .unwrap_or_else(PoisonError::into_inner) | ||
| .insert(id, slot.clone()); | ||
| } | ||
| SlotGuard(slot) | ||
| } |
There was a problem hiding this comment.
m: Following with the previous suggestion, this function likely also can be adjusted to take a concrete thread ID. The call site should be updated not to call the function when the thread ID is None.
| fn register(os_thread_id: Option<u64>) -> Self { | |
| let slot = Arc::new(Slot { | |
| os_thread_id, | |
| hub: RwLock::new(Weak::new()), | |
| }); | |
| if let Some(id) = os_thread_id { | |
| REGISTRY | |
| .lock() | |
| .unwrap_or_else(PoisonError::into_inner) | |
| .insert(id, slot.clone()); | |
| } | |
| SlotGuard(slot) | |
| } | |
| fn register(os_thread_id: u64) -> Self { | |
| let slot = Arc::new(Slot { | |
| os_thread_id, | |
| hub: RwLock::new(Weak::new()), | |
| }); | |
| REGISTRY | |
| .lock() | |
| .unwrap_or_else(PoisonError::into_inner) | |
| .insert(id, slot.clone()); | |
| SlotGuard(slot) | |
| } |
| target_os = "macos", | ||
| windows | ||
| ))] | ||
| pub fn current_os_thread_id() -> Option<u64> { |
There was a problem hiding this comment.
m: Could we avoid making this function public?
Ignoring tests, the function appears to be only called once, and only on macOS, outside of sentry-core.
As this API is unlikely to be useful to end users, I would prefer we make it private (or pub(crate) if needed) and to simply replace the single macOS call in sentry-minidump with a direct call to libc::pthread_mach_thread_np(libc::pthread_self()). We can note in a comment on this method that sentry-minidump expects us to use exactly that value in the registry.
| pub fn current_os_thread_id() -> Option<u64> { | |
| fn current_os_thread_id() -> Option<u64> { |
| /// Returns the hub that is current on the thread with the given | ||
| /// operating system thread id. | ||
| /// | ||
| /// The id is the one [`current_os_thread_id`](crate::current_os_thread_id) | ||
| /// returns on that thread. A thread is known once it has used | ||
| /// [`Hub::current`] or [`Hub::run`]; hubs switched with [`Hub::run`] | ||
| /// are tracked. Returns `None` for threads that never touched a hub | ||
| /// or have exited. | ||
| /// | ||
| /// This is meant for crash reporters that learn the crashing thread | ||
| /// from the OS and want its scope. | ||
| #[cfg(feature = "thread-registry")] | ||
| pub fn for_os_thread(os_thread_id: u64) -> Option<Arc<Hub>> { | ||
| crate::thread_registry::hub_for_os_thread(os_thread_id) | ||
| } | ||
|
|
||
| /// Returns a copy of the hub's current scope. | ||
| /// | ||
| /// Unlike [`Hub::configure_scope`], this only takes the hub's read | ||
| /// lock, so it does not wait for other threads that read the scope. | ||
| #[cfg(feature = "thread-registry")] | ||
| pub fn scope_snapshot(&self) -> Scope { | ||
| self.with_current_scope(Scope::clone) | ||
| } | ||
|
|
There was a problem hiding this comment.
m: To reduce public API surface, we can replace these two functions with a single function that gets the scope for a given OS thread.
| /// Returns the hub that is current on the thread with the given | |
| /// operating system thread id. | |
| /// | |
| /// The id is the one [`current_os_thread_id`](crate::current_os_thread_id) | |
| /// returns on that thread. A thread is known once it has used | |
| /// [`Hub::current`] or [`Hub::run`]; hubs switched with [`Hub::run`] | |
| /// are tracked. Returns `None` for threads that never touched a hub | |
| /// or have exited. | |
| /// | |
| /// This is meant for crash reporters that learn the crashing thread | |
| /// from the OS and want its scope. | |
| #[cfg(feature = "thread-registry")] | |
| pub fn for_os_thread(os_thread_id: u64) -> Option<Arc<Hub>> { | |
| crate::thread_registry::hub_for_os_thread(os_thread_id) | |
| } | |
| /// Returns a copy of the hub's current scope. | |
| /// | |
| /// Unlike [`Hub::configure_scope`], this only takes the hub's read | |
| /// lock, so it does not wait for other threads that read the scope. | |
| #[cfg(feature = "thread-registry")] | |
| pub fn scope_snapshot(&self) -> Scope { | |
| self.with_current_scope(Scope::clone) | |
| } | |
| /// Returns the scope of the hub that is current on the thread with the | |
| /// given operating system thread id. | |
| /// | |
| /// The id is the one a crash handler reports for the crashing thread. | |
| /// A thread that is unknown, because it never used a hub or has exited, | |
| /// gets the main hub's scope, which is the scope such a thread inherits | |
| /// when it first uses [`Hub::current`]. | |
| #[cfg(feature = "thread-registry")] | |
| pub fn scope_for_os_thread(os_thread_id: u64) -> Arc<Scope> { | |
| let hub = Hub::for_os_thread(os_thread_id).unwrap_or_else(Hub::main); | |
| hub.inner.with(|stack| stack.top().scope.clone()) | |
| } | |
As an added benefit, the call site in sentry-minidump would be much simpler, and we also avoid cloning the Scope by Arc-cloning instead.
| pub(crate) const MSG_SCOPE_CHUNK: u32 = 1; | ||
| pub(crate) const MSG_SCOPE_END: u32 = 2; |
There was a problem hiding this comment.
l: MSG_SCOPE_UPDATE, with its value of 0, is still defined in lib.rs, even though the values are sent over the same channel.
Can we move its definition here for consistency and clarity?
|
|
||
| /// The reporter reads one message in one call and does not reassemble | ||
| /// partial reads on every platform, so keep each message small. | ||
| const CHUNK_SIZE: usize = 16 * 1024; |
There was a problem hiding this comment.
h: This appears to be too high on macOS; the maximum stream size appears to be 8 KiB on macOS, so a scope that is bigger than 8 KiB causes a timeout when attempting to gather the scope in the crash reporter.
Please reduce this and also add an e2e test which uses a scope bigger than this limit to make sure that all supported platforms (Linux, Windows, and macOS) support the limit.
| //! Scope changes do not cross the process boundary on their own. Send them | ||
| //! to the crash reporter through the integration: | ||
| //! The crash event carries the scope of the thread that crashed: user, | ||
| //! tags, extra, contexts, breadcrumbs, level, transaction and fingerprint, |
There was a problem hiding this comment.
m: We appear to always use Level::FATAL. I believe that to be more reasonable than using the scope's level, so we should update the docs here accordingly not to claim that we respect the scope level.
| //! tags, extra, contexts, breadcrumbs, level, transaction and fingerprint, | |
| //! tags, extra, contexts, breadcrumbs, transaction and fingerprint, |
| #[cfg(feature = "thread-registry")] | ||
| crate::thread_registry::set_current_hub(&hub); |
There was a problem hiding this comment.
m: This likely adds some overhead to each hub switch. Have you done any benchmarking to measure how much the additional overhead is?
If not, that may be something worthwhile prior to the merge.
There was a problem hiding this comment.
Hub::run without thread-registry feature: 3.6 ns
Hub::run with thread-registry feature: 11.0 ns
Shall I put the scope synchronisation behind a minidump-scope feature so it's possible to capture minidumps without scope and without this performance impact?
This branch has not been deployed
2 participants
The minidump crash event now carries the scope of the thread that crashed, including hubs bound with
Hub::run, so a server with one hub per request reports the request that crashed.Nothing is sent to the crash reporter until the crash. The new
on_crashhook inminidumper-childnames the crashing OS thread, a parked helper thread serializes that thread's scope withScope::apply_to_event, and the handler sends it before the dump request. The reporter merges it with the fatal event. This makes the manualset_user/set_tag/set_extra/add_breadcrumbonMinidumpIntegrationunnecessary, so they are deprecated. They still work as before: each call sends a scope update to the reporter, which applies it to its hub, and the crash event merges those values with the crashing thread's scope. This keeps the change non-breaking.sentry-coregains athread-registryfeature:Hub::for_os_threadandcurrent_os_thread_idmap OS thread ids to the hub current on each thread, kept up to date bySwitchGuard, andHub::scope_snapshotcopies a hub's scope under the read lock only. Zero cost with the feature off.Crash handler safety
When the handler runs, the crashed thread may hold the allocator, registry or scope lock. So on every platform the handler allocates nothing and takes no lock that the app can hold: it sets atomics,
unparks the helper (one wake syscall, no allocation), polls an atomicdoneflag with 1 ms sleeps, then reads the result withtry_lock(which cannot block, because the helper unlocks before it setsdone) and writes it to the socket.Everything else runs on the
sentry-minidump-scopehelper thread: registry lookup, scope copy, event processors, JSON. It copies the scope withHub::scope_snapshot, notconfigure_scope, becauseconfigure_scopealso writes the scope back under the write lock and would block if the crashed thread held the read lock. It starts at init, because spawning a thread in the handler would allocate. If the crashed thread holds a lock the helper needs, the helper blocks, the handler's wait hitsscope_timeout(default 2s) and the minidump event goes out without scope. The worst case is a missing scope, never a missing minidump.On Linux and Windows the handler runs on the crashing thread (signal handler / exception handler).
macOS: resuming the helper
crash-handleruses a Mach exception port on macOS. The callback runs on a dedicated handler thread, not the crashing thread, andSIGABRTtakes the same path. Before it calls the callback, the handler suspends every other thread, the helper included. So the helper stores its Mach port at startup,startwaits until it has, andon_crashcallsthread_resumeon it before it wakes it. After that, macOS uses the same wait and timeout as Linux and Windows.This is the crate's one
unsafecall, throughmach2(already in the lockfile throughcrash-handler). Whencrash-handlerresumes all threads after the callback, its call on the helper fails harmlessly.Tested on macOS: a crash inside
add_breadcrumb, which holds the hub write lock, hung forever with the earlier inline serialization. Now the minidump arrives afterscope_timeout, without scope. A crash right aftersentry::initgets its minidump in 0.1 to 0.5 s, so the helper is resumed without waiting for the timeout.Details
gettid/tidon Linux,GetCurrentThreadId/thread_idon Windows, and the Mach port name (pthread_mach_thread_np/thread) on macOS. Threads that never used Sentry fall back to the main hub.Eventin 16 KiB chunks plus an end message, because the reporter reads one message per call. Bad or missing JSON gives an empty event. Attachments on the scope are not carried.Tested on Linux and macOS so far. I wont be back with my Windows machine for a week!