diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1068e7d7..9442040478 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ ### Fixes - 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)) +- Fix thread leak caused by eager creation of `SentryExecutorService` in `SentryOptions` ([#5093](https://github.com/getsentry/sentry-java/pull/5093)) + - There were cases where we created options that ended up unused but we failed to clean those up. ### Dependencies diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 600fe404fb..69b3982af8 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -41,7 +41,7 @@ public final class io/sentry/android/core/ActivityLifecycleIntegration : android } public class io/sentry/android/core/AndroidContinuousProfiler : io/sentry/IContinuousProfiler, io/sentry/transport/RateLimiter$IRateLimitObserver { - public fun (Lio/sentry/android/core/BuildInfoProvider;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ILogger;Ljava/lang/String;ILio/sentry/ISentryExecutorService;)V + public fun (Lio/sentry/android/core/BuildInfoProvider;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ILogger;Ljava/lang/String;ILio/sentry/util/LazyEvaluator$Evaluator;)V public fun close (Z)V public fun getChunkId ()Lio/sentry/protocol/SentryId; public fun getProfilerId ()Lio/sentry/protocol/SentryId; @@ -114,7 +114,7 @@ public final class io/sentry/android/core/AndroidMetricsBatchProcessorFactory : public class io/sentry/android/core/AndroidProfiler { protected final field lock Lio/sentry/util/AutoClosableReentrantLock; - public fun (Ljava/lang/String;ILio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ISentryExecutorService;Lio/sentry/ILogger;)V + public fun (Ljava/lang/String;ILio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/util/LazyEvaluator$Evaluator;Lio/sentry/ILogger;)V public fun close ()V public fun endAndCollect (ZLjava/util/List;)Lio/sentry/android/core/AndroidProfiler$ProfileEndData; public fun start ()Lio/sentry/android/core/AndroidProfiler$ProfileStartData; diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java index f567770c20..41362c9d93 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidContinuousProfiler.java @@ -26,6 +26,7 @@ import io.sentry.protocol.SentryId; import io.sentry.transport.RateLimiter; import io.sentry.util.AutoClosableReentrantLock; +import io.sentry.util.LazyEvaluator; import io.sentry.util.SentryRandom; import java.util.ArrayList; import java.util.List; @@ -45,7 +46,7 @@ public class AndroidContinuousProfiler private final @NotNull ILogger logger; private final @Nullable String profilingTracesDirPath; private final int profilingTracesHz; - private final @NotNull ISentryExecutorService executorService; + private final @NotNull LazyEvaluator.Evaluator executorServiceSupplier; private final @NotNull BuildInfoProvider buildInfoProvider; private boolean isInitialized = false; private final @NotNull SentryFrameMetricsCollector frameMetricsCollector; @@ -73,13 +74,13 @@ public AndroidContinuousProfiler( final @NotNull ILogger logger, final @Nullable String profilingTracesDirPath, final int profilingTracesHz, - final @NotNull ISentryExecutorService executorService) { + final @NotNull LazyEvaluator.Evaluator executorServiceSupplier) { this.logger = logger; this.frameMetricsCollector = frameMetricsCollector; this.buildInfoProvider = buildInfoProvider; this.profilingTracesDirPath = profilingTracesDirPath; this.profilingTracesHz = profilingTracesHz; - this.executorService = executorService; + this.executorServiceSupplier = executorServiceSupplier; } private void init() { @@ -222,7 +223,8 @@ private void start() { } try { - stopFuture = executorService.schedule(() -> stop(true), MAX_CHUNK_DURATION_MILLIS); + stopFuture = + executorServiceSupplier.evaluate().schedule(() -> stop(true), MAX_CHUNK_DURATION_MILLIS); } catch (RejectedExecutionException e) { logger.log( SentryLevel.ERROR, diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java index de42e13ee2..81ac3b35d6 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java @@ -343,7 +343,7 @@ private static void setupProfiler( options.getLogger(), options.getProfilingTracesDirPath(), options.getProfilingTracesHz(), - options.getExecutorService())); + () -> options.getExecutorService())); } } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java index 2ae1d0d3db..f4357b010f 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidProfiler.java @@ -16,6 +16,7 @@ import io.sentry.profilemeasurements.ProfileMeasurement; import io.sentry.profilemeasurements.ProfileMeasurementValue; import io.sentry.util.AutoClosableReentrantLock; +import io.sentry.util.LazyEvaluator; import io.sentry.util.Objects; import java.io.File; import java.util.ArrayDeque; @@ -92,7 +93,8 @@ public ProfileEndData( private final @NotNull ArrayDeque frozenFrameRenderMeasurements = new ArrayDeque<>(); private final @NotNull Map measurementsMap = new HashMap<>(); - private final @Nullable ISentryExecutorService timeoutExecutorService; + private final @Nullable LazyEvaluator.Evaluator + timeoutExecutorServiceSupplier; private final @NotNull ILogger logger; private volatile boolean isRunning = false; protected final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); @@ -101,14 +103,15 @@ public AndroidProfiler( final @NotNull String tracesFilesDirPath, final int intervalUs, final @NotNull SentryFrameMetricsCollector frameMetricsCollector, - final @Nullable ISentryExecutorService timeoutExecutorService, + final @Nullable LazyEvaluator.Evaluator + timeoutExecutorServiceSupplier, final @NotNull ILogger logger) { this.traceFilesDir = new File(Objects.requireNonNull(tracesFilesDirPath, "TracesFilesDirPath is required")); this.intervalUs = intervalUs; this.logger = Objects.requireNonNull(logger, "Logger is required"); // Timeout executor is nullable, as timeouts will not be there for continuous profiling - this.timeoutExecutorService = timeoutExecutorService; + this.timeoutExecutorServiceSupplier = timeoutExecutorServiceSupplier; this.frameMetricsCollector = Objects.requireNonNull(frameMetricsCollector, "SentryFrameMetricsCollector is required"); } @@ -185,10 +188,11 @@ public void onFrameMetricCollected( // We stop profiling after a timeout to avoid huge profiles to be sent try { - if (timeoutExecutorService != null) { + if (timeoutExecutorServiceSupplier != null) { scheduledFinish = - timeoutExecutorService.schedule( - () -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS); + timeoutExecutorServiceSupplier + .evaluate() + .schedule(() -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS); } } catch (RejectedExecutionException e) { logger.log( diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java index a6d07ba069..e44ed746a0 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java @@ -20,6 +20,7 @@ import io.sentry.android.core.internal.util.CpuInfoUtils; import io.sentry.android.core.internal.util.SentryFrameMetricsCollector; import io.sentry.util.AutoClosableReentrantLock; +import io.sentry.util.LazyEvaluator; import io.sentry.util.Objects; import java.util.ArrayList; import java.util.Date; @@ -34,7 +35,7 @@ final class AndroidTransactionProfiler implements ITransactionProfiler { private final @Nullable String profilingTracesDirPath; private final boolean isProfilingEnabled; private final int profilingTracesHz; - private final @NotNull ISentryExecutorService executorService; + private final @NotNull LazyEvaluator.Evaluator executorServiceSupplier; private final @NotNull BuildInfoProvider buildInfoProvider; private boolean isInitialized = false; private final @NotNull AtomicBoolean isRunning = new AtomicBoolean(false); @@ -65,7 +66,7 @@ public AndroidTransactionProfiler( sentryAndroidOptions.getProfilingTracesDirPath(), sentryAndroidOptions.isProfilingEnabled(), sentryAndroidOptions.getProfilingTracesHz(), - sentryAndroidOptions.getExecutorService()); + () -> sentryAndroidOptions.getExecutorService()); } public AndroidTransactionProfiler( @@ -77,6 +78,26 @@ public AndroidTransactionProfiler( final boolean isProfilingEnabled, final int profilingTracesHz, final @NotNull ISentryExecutorService executorService) { + this( + context, + buildInfoProvider, + frameMetricsCollector, + logger, + profilingTracesDirPath, + isProfilingEnabled, + profilingTracesHz, + () -> executorService); + } + + public AndroidTransactionProfiler( + final @NotNull Context context, + final @NotNull BuildInfoProvider buildInfoProvider, + final @NotNull SentryFrameMetricsCollector frameMetricsCollector, + final @NotNull ILogger logger, + final @Nullable String profilingTracesDirPath, + final boolean isProfilingEnabled, + final int profilingTracesHz, + final @NotNull LazyEvaluator.Evaluator executorServiceSupplier) { this.context = Objects.requireNonNull( ContextUtils.getApplicationContext(context), "The application context is required"); @@ -88,8 +109,9 @@ public AndroidTransactionProfiler( this.profilingTracesDirPath = profilingTracesDirPath; this.isProfilingEnabled = isProfilingEnabled; this.profilingTracesHz = profilingTracesHz; - this.executorService = - Objects.requireNonNull(executorService, "The ISentryExecutorService is required."); + this.executorServiceSupplier = + Objects.requireNonNull( + executorServiceSupplier, "A supplier for ISentryExecutorService is required."); this.profileStartTimestamp = DateUtils.getCurrentDateTime(); } @@ -123,7 +145,7 @@ private void init() { profilingTracesDirPath, (int) SECONDS.toMicros(1) / profilingTracesHz, frameMetricsCollector, - executorService, + executorServiceSupplier, logger); } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java index 12ec55f8e1..d687670f9b 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java @@ -165,6 +165,7 @@ private void createAndStartContinuousProfiler( return; } + final @NotNull SentryExecutorService startupExecutorService = new SentryExecutorService(); final @NotNull IContinuousProfiler appStartContinuousProfiler = new AndroidContinuousProfiler( buildInfoProvider, @@ -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."); diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt index f85ad6ab51..162e56c36e 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidContinuousProfilerTest.kt @@ -98,7 +98,7 @@ class AndroidContinuousProfilerTest { options.logger, options.profilingTracesDirPath, options.profilingTracesHz, - options.executorService, + { options.executorService }, ) } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt index 921620c435..66bda9bce0 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidProfilerTest.kt @@ -12,6 +12,7 @@ import io.sentry.android.core.internal.util.SentryFrameMetricsCollector import io.sentry.profilemeasurements.ProfileMeasurement import io.sentry.test.getCtor import io.sentry.test.getProperty +import io.sentry.util.LazyEvaluator import java.io.File import java.util.concurrent.Callable import java.util.concurrent.Future @@ -44,7 +45,7 @@ class AndroidProfilerTest { String::class.java, Int::class.java, SentryFrameMetricsCollector::class.java, - ISentryExecutorService::class.java, + LazyEvaluator.Evaluator::class.java, ILogger::class.java, ) private val fixture = Fixture() @@ -100,7 +101,7 @@ class AndroidProfilerTest { options.profilingTracesDirPath!!, interval, frameMetricsCollector, - options.executorService, + { options.executorService }, options.logger, ) } @@ -154,19 +155,19 @@ class AndroidProfilerTest { assertFailsWith { ctor.newInstance( - arrayOf(null, 0, mock(), mock(), mock()) + arrayOf(null, 0, mock(), { mock() }, mock()) ) } assertFailsWith { ctor.newInstance( - arrayOf("mock", 0, null, mock(), mock()) + arrayOf("mock", 0, null, { mock() }, mock()) ) } assertFailsWith { ctor.newInstance(arrayOf("mock", 0, mock(), null, mock())) } assertFailsWith { - ctor.newInstance(arrayOf("mock", 0, mock(), mock(), null)) + ctor.newInstance(arrayOf("mock", 0, mock(), { mock() }, null)) } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt index b31c5c1fbf..df9ce60883 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SendCachedEnvelopeIntegrationTest.kt @@ -7,8 +7,8 @@ import io.sentry.IScopes import io.sentry.ISentryExecutorService import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory +import io.sentry.SentryExecutorService import io.sentry.SentryLevel.DEBUG -import io.sentry.SentryOptions import io.sentry.test.DeferredExecutorService import io.sentry.test.ImmediateExecutorService import io.sentry.transport.RateLimiter @@ -46,7 +46,7 @@ class SendCachedEnvelopeIntegrationTest { options.cacheDirPath = cacheDirPath options.setLogger(logger) options.isDebug = true - options.executorService = mockExecutorService ?: SentryOptions().executorService + options.executorService = mockExecutorService ?: SentryExecutorService() whenever(sender.send()).then { Thread.sleep(delaySend) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 70ff460f3b..0a14eaf5ee 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3440,6 +3440,7 @@ public class io/sentry/SentryOptions { public static final field MAX_EVENT_SIZE_BYTES J protected final field lock Lio/sentry/util/AutoClosableReentrantLock; public fun ()V + public fun activate ()V public fun addBundleId (Ljava/lang/String;)V public fun addContextTag (Ljava/lang/String;)V public fun addEventProcessor (Lio/sentry/EventProcessor;)V diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 178f97a3b3..ff84b15165 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -329,6 +329,8 @@ private static void init(final @NotNull SentryOptions options, final boolean glo "Sentry has been already initialized. Previous configuration will be overwritten."); } + options.activate(); + final IScopes scopes = getCurrentScopes(); scopes.close(true); diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 2b6634d0a1..14f4acf51f 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -646,6 +646,17 @@ public void setProfilerConverter(@NotNull IProfileConverter profilerConverter) { this.profilerConverter = profilerConverter; } + /** Starts expensive parts of the options during Sentry.init */ + @ApiStatus.Internal + public void activate() { + if (executorService instanceof NoOpSentryExecutorService) { + // SentryExecutorService should be initialized before any + // SendCachedEventFireAndForgetIntegration + executorService = new SentryExecutorService(this); + executorService.prewarm(); + } + } + /** * Configuration options for Sentry Build Distribution. NOTE: Ideally this would be in * SentryAndroidOptions, but there's a circular dependency issue between sentry-android-core and @@ -3322,10 +3333,6 @@ private SentryOptions(final boolean empty) { if (!empty) { setSpanFactory(SpanFactoryFactory.create(new LoadClass(), NoOpLogger.getInstance())); - // SentryExecutorService should be initialized before any - // SendCachedEventFireAndForgetIntegration - executorService = new SentryExecutorService(this); - executorService.prewarm(); // UncaughtExceptionHandlerIntegration should be inited before any other Integration. // if there's an error on the setup, we are able to capture it diff --git a/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt b/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt index 4a5faf6435..677f32441e 100644 --- a/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt +++ b/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt @@ -33,6 +33,7 @@ class SendCachedEnvelopeFireAndForgetIntegrationTest { options.setDebug(true) options.setLogger(logger) options.sdkVersion = SdkVersion("test", "1.2.3") + options.activate() } fun getSut(): SendCachedEnvelopeFireAndForgetIntegration = diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt index 3b8549ce00..c94375735d 100644 --- a/sentry/src/test/java/io/sentry/SentryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTest.kt @@ -776,9 +776,12 @@ class SentryTest { it.profilesSampleRate = 1.0 it.cacheDirPath = getTempPath() it.setLogger(logger) + it.executorService = SentryExecutorService() + it.executorService.prewarm() it.executorService.close(0) it.isDebug = true } + verify(logger) .log( eq(SentryLevel.ERROR), @@ -874,6 +877,7 @@ class SentryTest { it.release = "io.sentry.sample@1.1.0+220" it.environment = "debug" + it.executorService = SentryExecutorService() it.executorService.submit { // here the values should be still old. Sentry.init will submit another runnable // to notify the options observers, but because the executor is single-threaded, the @@ -942,6 +946,7 @@ class SentryTest { previousSessionFile.bufferedWriter(), ) + it.executorService = SentryExecutorService() it.executorService.submit { // here the previous session should still exist. Sentry.init will submit another runnable // to finalize the previous session, but because the executor is single-threaded, the