Skip to content

Commit b6cfb57

Browse files
adinaueradinauer
authored
Fix thread leak caused by eager creation of SentryExecutorService in SentryOptions (#5093)
* Fix thread leak from SentryExecutorService * use supplier ctor for AndroidTransactionProfiler * changelog
1 parent aefefa6 commit b6cfb57

File tree

15 files changed

+81
-32
lines changed

15 files changed

+81
-32
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
### Fixes
1010

1111
- When merging tombstones with Native SDK, use the tombstone message if the Native SDK didn't explicitly provide one. ([#5095](https://github.com/getsentry/sentry-java/pull/5095))
12+
- Fix thread leak caused by eager creation of `SentryExecutorService` in `SentryOptions` ([#5093](https://github.com/getsentry/sentry-java/pull/5093))
13+
- There were cases where we created options that ended up unused but we failed to clean those up.
1214

1315
### Dependencies
1416

‎sentry-android-core/api/sentry-android-core.api‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public final class io/sentry/android/core/ActivityLifecycleIntegration : android
4141
}
4242

4343
public class io/sentry/android/core/AndroidContinuousProfiler : io/sentry/IContinuousProfiler, io/sentry/transport/RateLimiter$IRateLimitObserver {
44-
public fun <init> (Lio/sentry/android/core/BuildInfoProvider;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ILogger;Ljava/lang/String;ILio/sentry/ISentryExecutorService;)V
44+
public fun <init> (Lio/sentry/android/core/BuildInfoProvider;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ILogger;Ljava/lang/String;ILio/sentry/util/LazyEvaluator$Evaluator;)V
4545
public fun close (Z)V
4646
public fun getChunkId ()Lio/sentry/protocol/SentryId;
4747
public fun getProfilerId ()Lio/sentry/protocol/SentryId;
@@ -114,7 +114,7 @@ public final class io/sentry/android/core/AndroidMetricsBatchProcessorFactory :
114114

115115
public class io/sentry/android/core/AndroidProfiler {
116116
protected final field lock Lio/sentry/util/AutoClosableReentrantLock;
117-
public fun <init> (Ljava/lang/String;ILio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ISentryExecutorService;Lio/sentry/ILogger;)V
117+
public fun <init> (Ljava/lang/String;ILio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/util/LazyEvaluator$Evaluator;Lio/sentry/ILogger;)V
118118
public fun close ()V
119119
public fun endAndCollect (ZLjava/util/List;)Lio/sentry/android/core/AndroidProfiler$ProfileEndData;
120120
public fun start ()Lio/sentry/android/core/AndroidProfiler$ProfileStartData;

‎sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import io.sentry.protocol.SentryId;
2727
import io.sentry.transport.RateLimiter;
2828
import io.sentry.util.AutoClosableReentrantLock;
29+
import io.sentry.util.LazyEvaluator;
2930
import io.sentry.util.SentryRandom;
3031
import java.util.ArrayList;
3132
import java.util.List;
@@ -45,7 +46,7 @@ public class AndroidContinuousProfiler
4546
private final @NotNull ILogger logger;
4647
private final @Nullable String profilingTracesDirPath;
4748
private final int profilingTracesHz;
48-
private final @NotNull ISentryExecutorService executorService;
49+
private final @NotNull LazyEvaluator.Evaluator<ISentryExecutorService> executorServiceSupplier;
4950
private final @NotNull BuildInfoProvider buildInfoProvider;
5051
private boolean isInitialized = false;
5152
private final @NotNull SentryFrameMetricsCollector frameMetricsCollector;
@@ -73,13 +74,13 @@ public AndroidContinuousProfiler(
7374
final @NotNull ILogger logger,
7475
final @Nullable String profilingTracesDirPath,
7576
final int profilingTracesHz,
76-
final @NotNull ISentryExecutorService executorService) {
77+
final @NotNull LazyEvaluator.Evaluator<ISentryExecutorService> executorServiceSupplier) {
7778
this.logger = logger;
7879
this.frameMetricsCollector = frameMetricsCollector;
7980
this.buildInfoProvider = buildInfoProvider;
8081
this.profilingTracesDirPath = profilingTracesDirPath;
8182
this.profilingTracesHz = profilingTracesHz;
82-
this.executorService = executorService;
83+
this.executorServiceSupplier = executorServiceSupplier;
8384
}
8485

8586
private void init() {
@@ -222,7 +223,8 @@ private void start() {
222223
}
223224

224225
try {
225-
stopFuture = executorService.schedule(() -> stop(true), MAX_CHUNK_DURATION_MILLIS);
226+
stopFuture =
227+
executorServiceSupplier.evaluate().schedule(() -> stop(true), MAX_CHUNK_DURATION_MILLIS);
226228
} catch (RejectedExecutionException e) {
227229
logger.log(
228230
SentryLevel.ERROR,

‎sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ private static void setupProfiler(
343343
options.getLogger(),
344344
options.getProfilingTracesDirPath(),
345345
options.getProfilingTracesHz(),
346-
options.getExecutorService()));
346+
() -> options.getExecutorService()));
347347
}
348348
}
349349
}

‎sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import io.sentry.profilemeasurements.ProfileMeasurement;
1717
import io.sentry.profilemeasurements.ProfileMeasurementValue;
1818
import io.sentry.util.AutoClosableReentrantLock;
19+
import io.sentry.util.LazyEvaluator;
1920
import io.sentry.util.Objects;
2021
import java.io.File;
2122
import java.util.ArrayDeque;
@@ -92,7 +93,8 @@ public ProfileEndData(
9293
private final @NotNull ArrayDeque<ProfileMeasurementValue> frozenFrameRenderMeasurements =
9394
new ArrayDeque<>();
9495
private final @NotNull Map<String, ProfileMeasurement> measurementsMap = new HashMap<>();
95-
private final @Nullable ISentryExecutorService timeoutExecutorService;
96+
private final @Nullable LazyEvaluator.Evaluator<ISentryExecutorService>
97+
timeoutExecutorServiceSupplier;
9698
private final @NotNull ILogger logger;
9799
private volatile boolean isRunning = false;
98100
protected final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
@@ -101,14 +103,15 @@ public AndroidProfiler(
101103
final @NotNull String tracesFilesDirPath,
102104
final int intervalUs,
103105
final @NotNull SentryFrameMetricsCollector frameMetricsCollector,
104-
final @Nullable ISentryExecutorService timeoutExecutorService,
106+
final @Nullable LazyEvaluator.Evaluator<ISentryExecutorService>
107+
timeoutExecutorServiceSupplier,
105108
final @NotNull ILogger logger) {
106109
this.traceFilesDir =
107110
new File(Objects.requireNonNull(tracesFilesDirPath, "TracesFilesDirPath is required"));
108111
this.intervalUs = intervalUs;
109112
this.logger = Objects.requireNonNull(logger, "Logger is required");
110113
// Timeout executor is nullable, as timeouts will not be there for continuous profiling
111-
this.timeoutExecutorService = timeoutExecutorService;
114+
this.timeoutExecutorServiceSupplier = timeoutExecutorServiceSupplier;
112115
this.frameMetricsCollector =
113116
Objects.requireNonNull(frameMetricsCollector, "SentryFrameMetricsCollector is required");
114117
}
@@ -185,10 +188,11 @@ public void onFrameMetricCollected(
185188

186189
// We stop profiling after a timeout to avoid huge profiles to be sent
187190
try {
188-
if (timeoutExecutorService != null) {
191+
if (timeoutExecutorServiceSupplier != null) {
189192
scheduledFinish =
190-
timeoutExecutorService.schedule(
191-
() -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS);
193+
timeoutExecutorServiceSupplier
194+
.evaluate()
195+
.schedule(() -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS);
192196
}
193197
} catch (RejectedExecutionException e) {
194198
logger.log(

‎sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import io.sentry.android.core.internal.util.CpuInfoUtils;
2121
import io.sentry.android.core.internal.util.SentryFrameMetricsCollector;
2222
import io.sentry.util.AutoClosableReentrantLock;
23+
import io.sentry.util.LazyEvaluator;
2324
import io.sentry.util.Objects;
2425
import java.util.ArrayList;
2526
import java.util.Date;
@@ -34,7 +35,7 @@ final class AndroidTransactionProfiler implements ITransactionProfiler {
3435
private final @Nullable String profilingTracesDirPath;
3536
private final boolean isProfilingEnabled;
3637
private final int profilingTracesHz;
37-
private final @NotNull ISentryExecutorService executorService;
38+
private final @NotNull LazyEvaluator.Evaluator<ISentryExecutorService> executorServiceSupplier;
3839
private final @NotNull BuildInfoProvider buildInfoProvider;
3940
private boolean isInitialized = false;
4041
private final @NotNull AtomicBoolean isRunning = new AtomicBoolean(false);
@@ -65,7 +66,7 @@ public AndroidTransactionProfiler(
6566
sentryAndroidOptions.getProfilingTracesDirPath(),
6667
sentryAndroidOptions.isProfilingEnabled(),
6768
sentryAndroidOptions.getProfilingTracesHz(),
68-
sentryAndroidOptions.getExecutorService());
69+
() -> sentryAndroidOptions.getExecutorService());
6970
}
7071

7172
public AndroidTransactionProfiler(
@@ -77,6 +78,26 @@ public AndroidTransactionProfiler(
7778
final boolean isProfilingEnabled,
7879
final int profilingTracesHz,
7980
final @NotNull ISentryExecutorService executorService) {
81+
this(
82+
context,
83+
buildInfoProvider,
84+
frameMetricsCollector,
85+
logger,
86+
profilingTracesDirPath,
87+
isProfilingEnabled,
88+
profilingTracesHz,
89+
() -> executorService);
90+
}
91+
92+
public AndroidTransactionProfiler(
93+
final @NotNull Context context,
94+
final @NotNull BuildInfoProvider buildInfoProvider,
95+
final @NotNull SentryFrameMetricsCollector frameMetricsCollector,
96+
final @NotNull ILogger logger,
97+
final @Nullable String profilingTracesDirPath,
98+
final boolean isProfilingEnabled,
99+
final int profilingTracesHz,
100+
final @NotNull LazyEvaluator.Evaluator<ISentryExecutorService> executorServiceSupplier) {
80101
this.context =
81102
Objects.requireNonNull(
82103
ContextUtils.getApplicationContext(context), "The application context is required");
@@ -88,8 +109,9 @@ public AndroidTransactionProfiler(
88109
this.profilingTracesDirPath = profilingTracesDirPath;
89110
this.isProfilingEnabled = isProfilingEnabled;
90111
this.profilingTracesHz = profilingTracesHz;
91-
this.executorService =
92-
Objects.requireNonNull(executorService, "The ISentryExecutorService is required.");
112+
this.executorServiceSupplier =
113+
Objects.requireNonNull(
114+
executorServiceSupplier, "A supplier for ISentryExecutorService is required.");
93115
this.profileStartTimestamp = DateUtils.getCurrentDateTime();
94116
}
95117

@@ -123,7 +145,7 @@ private void init() {
123145
profilingTracesDirPath,
124146
(int) SECONDS.toMicros(1) / profilingTracesHz,
125147
frameMetricsCollector,
126-
executorService,
148+
executorServiceSupplier,
127149
logger);
128150
}
129151

‎sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ private void createAndStartContinuousProfiler(
165165
return;
166166
}
167167

168+
final @NotNull SentryExecutorService startupExecutorService = new SentryExecutorService();
168169
final @NotNull IContinuousProfiler appStartContinuousProfiler =
169170
new AndroidContinuousProfiler(
170171
buildInfoProvider,
@@ -173,7 +174,7 @@ private void createAndStartContinuousProfiler(
173174
logger,
174175
profilingOptions.getProfilingTracesDirPath(),
175176
profilingOptions.getProfilingTracesHz(),
176-
new SentryExecutorService());
177+
() -> startupExecutorService);
177178
appStartMetrics.setAppStartProfiler(null);
178179
appStartMetrics.setAppStartContinuousProfiler(appStartContinuousProfiler);
179180
logger.log(SentryLevel.DEBUG, "App start continuous profiling started.");
@@ -203,6 +204,7 @@ private void createAndStartTransactionProfiler(
203204
return;
204205
}
205206

207+
final @NotNull SentryExecutorService executorService = new SentryExecutorService();
206208
final @NotNull ITransactionProfiler appStartProfiler =
207209
new AndroidTransactionProfiler(
208210
context,
@@ -212,7 +214,7 @@ private void createAndStartTransactionProfiler(
212214
profilingOptions.getProfilingTracesDirPath(),
213215
profilingOptions.isProfilingEnabled(),
214216
profilingOptions.getProfilingTracesHz(),
215-
new SentryExecutorService());
217+
() -> executorService);
216218
appStartMetrics.setAppStartContinuousProfiler(null);
217219
appStartMetrics.setAppStartProfiler(appStartProfiler);
218220
logger.log(SentryLevel.DEBUG, "App start profiling started.");

‎sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ class AndroidContinuousProfilerTest {
9898
options.logger,
9999
options.profilingTracesDirPath,
100100
options.profilingTracesHz,
101-
options.executorService,
101+
{ options.executorService },
102102
)
103103
}
104104
}

‎sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import io.sentry.android.core.internal.util.SentryFrameMetricsCollector
1212
import io.sentry.profilemeasurements.ProfileMeasurement
1313
import io.sentry.test.getCtor
1414
import io.sentry.test.getProperty
15+
import io.sentry.util.LazyEvaluator
1516
import java.io.File
1617
import java.util.concurrent.Callable
1718
import java.util.concurrent.Future
@@ -44,7 +45,7 @@ class AndroidProfilerTest {
4445
String::class.java,
4546
Int::class.java,
4647
SentryFrameMetricsCollector::class.java,
47-
ISentryExecutorService::class.java,
48+
LazyEvaluator.Evaluator::class.java,
4849
ILogger::class.java,
4950
)
5051
private val fixture = Fixture()
@@ -100,7 +101,7 @@ class AndroidProfilerTest {
100101
options.profilingTracesDirPath!!,
101102
interval,
102103
frameMetricsCollector,
103-
options.executorService,
104+
{ options.executorService },
104105
options.logger,
105106
)
106107
}
@@ -154,19 +155,19 @@ class AndroidProfilerTest {
154155

155156
assertFailsWith<IllegalArgumentException> {
156157
ctor.newInstance(
157-
arrayOf(null, 0, mock(), mock<SentryExecutorService>(), mock<AndroidLogger>())
158+
arrayOf(null, 0, mock(), { mock<SentryExecutorService>() }, mock<AndroidLogger>())
158159
)
159160
}
160161
assertFailsWith<IllegalArgumentException> {
161162
ctor.newInstance(
162-
arrayOf("mock", 0, null, mock<SentryExecutorService>(), mock<AndroidLogger>())
163+
arrayOf("mock", 0, null, { mock<SentryExecutorService>() }, mock<AndroidLogger>())
163164
)
164165
}
165166
assertFailsWith<IllegalArgumentException> {
166167
ctor.newInstance(arrayOf("mock", 0, mock(), null, mock<AndroidLogger>()))
167168
}
168169
assertFailsWith<IllegalArgumentException> {
169-
ctor.newInstance(arrayOf("mock", 0, mock(), mock<SentryExecutorService>(), null))
170+
ctor.newInstance(arrayOf("mock", 0, mock(), { mock<SentryExecutorService>() }, null))
170171
}
171172
}
172173

‎sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ import io.sentry.IScopes
77
import io.sentry.ISentryExecutorService
88
import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget
99
import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory
10+
import io.sentry.SentryExecutorService
1011
import io.sentry.SentryLevel.DEBUG
11-
import io.sentry.SentryOptions
1212
import io.sentry.test.DeferredExecutorService
1313
import io.sentry.test.ImmediateExecutorService
1414
import io.sentry.transport.RateLimiter
@@ -46,7 +46,7 @@ class SendCachedEnvelopeIntegrationTest {
4646
options.cacheDirPath = cacheDirPath
4747
options.setLogger(logger)
4848
options.isDebug = true
49-
options.executorService = mockExecutorService ?: SentryOptions().executorService
49+
options.executorService = mockExecutorService ?: SentryExecutorService()
5050

5151
whenever(sender.send()).then {
5252
Thread.sleep(delaySend)

0 commit comments

Comments
 (0)