sentry-cocoa icon indicating copy to clipboard operation
sentry-cocoa copied to clipboard

Profiler should not suspend mach threads

Open philipphofmann opened this issue 2 years ago • 1 comments

enumerateBacktracesForAllThreads uses ThreadHandle::allExcludingCurrent() for building backtraces and suspends all threads except the current one.

https://github.com/getsentry/sentry-cocoa/blob/06548c0f88eefd4a54851752c7d33f4270e34ba9/Sources/Sentry/SentryBacktrace.cpp#L102-L105

This code also suspends the mach threads of SentryCrash, which are not suspended when SentryCrash suspends all threads.

https://github.com/getsentry/sentry-cocoa/blob/06548c0f88eefd4a54851752c7d33f4270e34ba9/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext.c#L163-L167

I think it's not a good idea to suspend the mach threads in the profiler. @armcknight, I think we should also use sentrycrashcm_isReservedThread in enumerateBacktracesForAllThreads and ignore the mach threads.

philipphofmann avatar Mar 17 '23 08:03 philipphofmann

This makes sense. We shouldn't even surface that thread in profiles, so makes no sense to suspend it.

armcknight avatar Mar 28 '23 03:03 armcknight

@philipphofmann can you tell me how you observed the mach exception server thread being profiled? I'm not seeing any threads for which sentrycrashcm_isReservedThread returns true in that part of the code when I added it to SentryBacktrace.cpp. Maybe task_info doesn't return reserved threads?

I'll close this, but feel free to reopen if I'm wrong here.

armcknight avatar Jun 18 '24 21:06 armcknight