-
-
Notifications
You must be signed in to change notification settings - Fork 465
Fix thread leak caused by eager creation of SentryExecutorService in SentryOptions
#5093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -165,6 +165,7 @@ private void createAndStartContinuousProfiler( | |
| return; | ||
| } | ||
|
|
||
| final @NotNull SentryExecutorService startupExecutorService = new SentryExecutorService(); | ||
| final @NotNull IContinuousProfiler appStartContinuousProfiler = | ||
| new AndroidContinuousProfiler( | ||
| buildInfoProvider, | ||
|
Comment on lines
165
to
171
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Suggested FixInstead of creating new Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We accept this edge case for now. |
||
|
|
@@ -173,7 +174,7 @@ private void createAndStartContinuousProfiler( | |
| logger, | ||
| profilingOptions.getProfilingTracesDirPath(), | ||
| profilingOptions.getProfilingTracesHz(), | ||
| new SentryExecutorService()); | ||
| () -> startupExecutorService); | ||
| appStartMetrics.setAppStartProfiler(null); | ||
| appStartMetrics.setAppStartContinuousProfiler(appStartContinuousProfiler); | ||
| logger.log(SentryLevel.DEBUG, "App start continuous profiling started."); | ||
|
|
@@ -203,6 +204,7 @@ private void createAndStartTransactionProfiler( | |
| return; | ||
| } | ||
|
|
||
| final @NotNull SentryExecutorService executorService = new SentryExecutorService(); | ||
| final @NotNull ITransactionProfiler appStartProfiler = | ||
| new AndroidTransactionProfiler( | ||
| context, | ||
|
|
@@ -212,7 +214,7 @@ private void createAndStartTransactionProfiler( | |
| profilingOptions.getProfilingTracesDirPath(), | ||
| profilingOptions.isProfilingEnabled(), | ||
| profilingOptions.getProfilingTracesHz(), | ||
| new SentryExecutorService()); | ||
| () -> executorService); | ||
| appStartMetrics.setAppStartContinuousProfiler(null); | ||
| appStartMetrics.setAppStartProfiler(appStartProfiler); | ||
| logger.log(SentryLevel.DEBUG, "App start profiling started."); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yeah, will change that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I created the ctor but didn't use it. It's being used now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.