Fix thread leak caused by eager creation of SentryExecutorService in SentryOptions
#5093
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
| return; | ||
| } | ||
|
|
||
| final @NotNull SentryExecutorService startupExecutorService = new SentryExecutorService(); |
There was a problem hiding this comment.
This creates a new thread for app startup profiling. My current understanding is that this is later reused and not just used for app startup profiling. This means we're not really leaking it since it ends up being used.
If there's cases where app startup profiler is abandoned, we'll need some sort of shutdown hook to also shut down the SentryExecutorService. LMK if I should implement that as well just to be safe here.
There was a problem hiding this comment.
I think it's fine as-is. Plus, afaik we'll need to deprecate/delete the transaction profiler soon (with the next major likely or span-only), so I wouldn't pour much into it
There was a problem hiding this comment.
ah I see this also affects continuous profiler too.. but I don't think we abandon it anywhere (unless process termination), so we hsould be good.
Btw, speaking of transaction profiler - I guess we should change it to accept a lambda, too (line 216)?
There was a problem hiding this comment.
ah I see this also affects continuous profiler too.. but I don't think we abandon it anywhere (unless process termination), so we hsould be good.
I'd assume once we get rid of transaction profiler, there wouldn't be much reason to abandon it.
I'll go through some things again to check for cases but I assume we can tackle those as a follow up since this isn't new behaviour but already existed previously.
Btw, speaking of transaction profiler - I guess we should change it to accept a lambda, too (line 216)?
Yeah, will change that too.
There was a problem hiding this comment.
Oops, I created the ctor but didn't use it. It's being used now.
There was a problem hiding this comment.
We agreed to ignore the edge case of an abandoned app start profiler for now since it only happens when switching between continuous and transaction profiling or when switching from enabled app startup profiling to disabled. And it'll only be leaking a single thread for a single app run. With plans to remove transaction based profiling in the future, this will become even more of an edge case.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| bbc35bb | 324.88 ms | 425.73 ms | 100.85 ms |
| d15471f | 307.28 ms | 381.85 ms | 74.57 ms |
| d364ace | 384.53 ms | 453.51 ms | 68.98 ms |
| ee747ae | 386.94 ms | 431.43 ms | 44.49 ms |
| 539ca63 | 313.51 ms | 355.43 ms | 41.92 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| b3d8889 | 371.69 ms | 432.96 ms | 61.26 ms |
| 3d205d0 | 352.15 ms | 432.53 ms | 80.38 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| bbc35bb | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| d364ace | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 539ca63 | 1.58 MiB | 2.12 MiB | 551.41 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.06 KiB |
| 3d205d0 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| private volatile boolean isRunning = false; | ||
| protected final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); | ||
|
|
||
| public AndroidProfiler( |
There was a problem hiding this comment.
a bit of a breaking change for RN, but should be fine since it's marked Internal anyway: https://github.com/getsentry/sentry-react-native/blob/74979ac1511f9110e88ba92af2f04ec892bb89e1/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java#L1018
There was a problem hiding this comment.
Thank you for the heads up @romtsn 🙇
As you said this seems more like an internal implementation breakage (which we should handle with the Android bump) and not a user facing thing.
romtsn
left a comment
romtsn
left a comment
There was a problem hiding this comment.
one point about transaction profiler, but lgtm otherwise!
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return; | ||
| } | ||
|
|
||
| final @NotNull SentryExecutorService startupExecutorService = new SentryExecutorService(); | ||
| final @NotNull IContinuousProfiler appStartContinuousProfiler = | ||
| new AndroidContinuousProfiler( | ||
| buildInfoProvider, |
There was a problem hiding this comment.
Bug: SentryPerformanceProvider eagerly creates SentryExecutorService instances at app startup, spawning threads before Sentry.init() is called and potentially causing thread leaks if profiling is disabled.
Severity: MEDIUM
Suggested Fix
Instead of creating new SentryExecutorService instances with new SentryExecutorService(), the profilers in SentryPerformanceProvider should obtain the executor service from SentryOptions. This can be achieved by passing a supplier that lazily gets the executor from the options once the SDK is initialized, ensuring thread creation is deferred until Sentry.init() and managed within the central SDK lifecycle.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java#L165-L171
Potential issue: In `SentryPerformanceProvider.java`, new `SentryExecutorService`
instances are created directly via `new SentryExecutorService()`. This constructor
immediately spawns a thread. Since `SentryPerformanceProvider` is a `ContentProvider`
that runs at app startup before `Sentry.init()`, these threads are created
unconditionally, bypassing the lazy initialization logic intended for the SDK. If app
start profiling is not enabled, these threads are created but never used, resulting in a
thread leak for the lifetime of the application process. Additionally, these executors
are created with `null` options, which prevents proper logging of rejected tasks.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
We accept this edge case for now.
#5093 (comment)
3 participants
📜 Description
When creating (non empty)
SentryOptionswe previously eagerly createdSentryExecutorServicewhich in turn creates a thread namedSentryExecutorServiceThreadFactory-0.There were cases where we created options that ended up unused but we failed to clean those up, e.g.:
SentryOptionsand due to lower priority of the call toSentry.initended up unusedSentryProperties(a subclass ofSentryOptions) were created even if the SDK wasn't enabled but we never cleaned up because the SDK was never enabled💡 Motivation and Context
Fixes #5079
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps