diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a290e2b3e..a3e054e715 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Performance + +- Schedule transaction idle/deadline timeouts on a shared, dedicated executor instead of spawning a `Timer` thread per transaction ([#5670](https://github.com/getsentry/sentry-java/pull/5670)) + ## 8.47.0 ### Behavioral Changes diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt index 8b842a0cfa..4b3368190e 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt @@ -23,6 +23,7 @@ import io.sentry.Scopes import io.sentry.Sentry import io.sentry.SentryDate import io.sentry.SentryDateProvider +import io.sentry.SentryExecutorService import io.sentry.SentryNanotimeDate import io.sentry.SentryTraceHeader import io.sentry.SentryTracer @@ -654,6 +655,8 @@ class ActivityLifecycleIntegrationTest { it.idleTimeout = 100 } ) + // the transaction idle timeout is scheduled on the dedicated timer executor + fixture.options.timerExecutorService = SentryExecutorService() sut.register(fixture.scopes, fixture.options) sut.onActivityCreated(activity, fixture.bundle) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 383ea92b11..ee70e5996e 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3702,6 +3702,7 @@ public class io/sentry/SentryOptions { public fun getSslSocketFactory ()Ljavax/net/ssl/SSLSocketFactory; public fun getTags ()Ljava/util/Map; public fun getThreadChecker ()Lio/sentry/util/thread/IThreadChecker; + public fun getTimerExecutorService ()Lio/sentry/ISentryExecutorService; public fun getTracePropagationTargets ()Ljava/util/List; public fun getTracesSampleRate ()Ljava/lang/Double; public fun getTracesSampler ()Lio/sentry/SentryOptions$TracesSamplerCallback; @@ -3866,6 +3867,7 @@ public class io/sentry/SentryOptions { public fun setStrictTraceContinuation (Z)V public fun setTag (Ljava/lang/String;Ljava/lang/String;)V public fun setThreadChecker (Lio/sentry/util/thread/IThreadChecker;)V + public fun setTimerExecutorService (Lio/sentry/ISentryExecutorService;)V public fun setTraceOptionsRequests (Z)V public fun setTracePropagationTargets (Ljava/util/List;)V public fun setTraceSampling (Z)V diff --git a/sentry/src/main/java/io/sentry/Scopes.java b/sentry/src/main/java/io/sentry/Scopes.java index 3b67b94916..936a331e3d 100644 --- a/sentry/src/main/java/io/sentry/Scopes.java +++ b/sentry/src/main/java/io/sentry/Scopes.java @@ -467,6 +467,12 @@ public void close(final boolean isRestarting) { getOptions().getContinuousProfiler().close(true); getOptions().getCompositePerformanceCollector().close(); getOptions().getConnectionStatusProvider().close(); + // On restart we intentionally leave the timer executor running so that pending idle/ + // deadline timeouts of transactions started before the restart still fire and finish + // those transactions. It self-terminates once idle (allowCoreThreadTimeOut). + if (!isRestarting) { + getOptions().getTimerExecutorService().close(getOptions().getShutdownTimeoutMillis()); + } final @NotNull ISentryExecutorService executorService = getOptions().getExecutorService(); if (isRestarting) { try { diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 919607e587..bf51a52c9e 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -354,6 +354,10 @@ private static void init(final @NotNull SentryOptions options, final boolean glo options.getExecutorService().prewarm(); } + if (options.getTimerExecutorService().isClosed()) { + options.setTimerExecutorService(new SentryExecutorService(options, true)); + } + // load lazy fields of the options in a separate thread try { options.getExecutorService().submit(() -> options.loadLazyFields()); diff --git a/sentry/src/main/java/io/sentry/SentryExecutorService.java b/sentry/src/main/java/io/sentry/SentryExecutorService.java index adb50b232e..ca4d1c0dd2 100644 --- a/sentry/src/main/java/io/sentry/SentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/SentryExecutorService.java @@ -49,6 +49,17 @@ public SentryExecutorService(final @Nullable SentryOptions options) { this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), options); } + SentryExecutorService(final @Nullable SentryOptions options, final boolean removeOnCancelPolicy) { + this(options); + // removes cancelled tasks from the work queue immediately instead of leaving them until their + // scheduled time; useful for executors that frequently reschedule (e.g. transaction timeouts) + executorService.setRemoveOnCancelPolicy(removeOnCancelPolicy); + // let the worker thread die when idle so an executor abandoned on SDK restart (its pending + // timeouts still fire) doesn't leak a live thread once its queue drains + executorService.setKeepAliveTime(10, TimeUnit.SECONDS); + executorService.allowCoreThreadTimeOut(true); + } + public SentryExecutorService() { this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), null); } diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 0d038482d0..e46592f1fe 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -317,6 +317,14 @@ public class SentryOptions { /** Sentry Executor Service that sends cached events and envelopes on App. start. */ private @NotNull ISentryExecutorService executorService = NoOpSentryExecutorService.getInstance(); + /** + * Dedicated executor for scheduling transaction idle/deadline timeouts. Kept separate from {@link + * #executorService} so timeout callbacks (which finish transactions) don't contend with cached + * event sending. + */ + private @NotNull ISentryExecutorService timerExecutorService = + NoOpSentryExecutorService.getInstance(); + /** * Whether SpotlightIntegration has already been loaded via reflection. This prevents re-adding it * if the user removed it in their configuration callback and activate() is called again. @@ -681,6 +689,13 @@ public void activate() { executorService.prewarm(); } + if (timerExecutorService instanceof NoOpSentryExecutorService) { + // Not prewarmed: its single worker thread is spawned lazily on the first scheduled timeout + // and then reused across all transactions. removeOnCancelPolicy keeps the work queue from + // accumulating cancelled timeouts (idle timers are cancelled and rescheduled per child span). + timerExecutorService = new SentryExecutorService(this, true); + } + // SpotlightIntegration is loaded via reflection to allow the sentry-spotlight module // to be excluded from release builds, preventing insecure HTTP URLs from appearing in APKs. // Only attempt once to avoid re-adding after user removal in their configuration callback. @@ -1568,6 +1583,30 @@ public void setExecutorService(final @NotNull ISentryExecutorService executorSer } } + /** + * Returns the dedicated executor used to schedule transaction idle/deadline timeouts. + * + * @return the timer executor service + */ + @ApiStatus.Internal + @NotNull + public ISentryExecutorService getTimerExecutorService() { + return timerExecutorService; + } + + /** + * Sets the dedicated executor used to schedule transaction idle/deadline timeouts. + * + * @param timerExecutorService the timer executor service + */ + @ApiStatus.Internal + @TestOnly + public void setTimerExecutorService(final @NotNull ISentryExecutorService timerExecutorService) { + if (timerExecutorService != null) { + this.timerExecutorService = timerExecutorService; + } + } + /** * Returns the connection timeout in milliseconds. * diff --git a/sentry/src/main/java/io/sentry/SentryTracer.java b/sentry/src/main/java/io/sentry/SentryTracer.java index 9729ac406b..97fbb5696e 100644 --- a/sentry/src/main/java/io/sentry/SentryTracer.java +++ b/sentry/src/main/java/io/sentry/SentryTracer.java @@ -12,9 +12,8 @@ import java.util.List; import java.util.ListIterator; import java.util.Map; -import java.util.Timer; -import java.util.TimerTask; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import org.jetbrains.annotations.ApiStatus; @@ -37,10 +36,13 @@ public final class SentryTracer implements ITransaction { */ private @NotNull FinishStatus finishStatus = FinishStatus.NOT_FINISHED; - private volatile @Nullable TimerTask idleTimeoutTask; - private volatile @Nullable TimerTask deadlineTimeoutTask; + private volatile @Nullable Future idleTimeoutFuture; + private volatile @Nullable Future deadlineTimeoutFuture; - private volatile @Nullable Timer timer = null; + // Shared executor used to schedule the timeout tasks. Null once the tracer is finished, at which + // point no more timeouts may be scheduled. It is never shut down here since it is shared + // SDK-wide. + private volatile @Nullable ISentryExecutorService timerExecutorService = null; private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock(); private final @NotNull AutoClosableReentrantLock tracerLock = new AutoClosableReentrantLock(); @@ -99,7 +101,7 @@ public SentryTracer( if (transactionOptions.getIdleTimeout() != null || transactionOptions.getDeadlineTimeout() != null) { - timer = new Timer(true); + timerExecutorService = scopes.getOptions().getTimerExecutorService(); scheduleDeadlineTimeout(); scheduleFinish(); @@ -109,22 +111,16 @@ public SentryTracer( @Override public void scheduleFinish() { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (timer != null) { + if (timerExecutorService != null) { final @Nullable Long idleTimeout = transactionOptions.getIdleTimeout(); if (idleTimeout != null) { cancelIdleTimer(); isIdleFinishTimerRunning.set(true); - idleTimeoutTask = - new TimerTask() { - @Override - public void run() { - onIdleTimeoutReached(); - } - }; try { - timer.schedule(idleTimeoutTask, idleTimeout); + idleTimeoutFuture = + timerExecutorService.schedule(this::onIdleTimeoutReached, idleTimeout); } catch (Throwable e) { scopes .getOptions() @@ -265,13 +261,12 @@ public void finish( }); final SentryTransaction transaction = new SentryTransaction(this); - if (timer != null) { + if (timerExecutorService != null) { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (timer != null) { + if (timerExecutorService != null) { cancelIdleTimer(); cancelDeadlineTimer(); - timer.cancel(); - timer = null; + timerExecutorService = null; } } } @@ -295,10 +290,10 @@ public void finish( private void cancelIdleTimer() { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (idleTimeoutTask != null) { - idleTimeoutTask.cancel(); + if (idleTimeoutFuture != null) { + idleTimeoutFuture.cancel(false); isIdleFinishTimerRunning.set(false); - idleTimeoutTask = null; + idleTimeoutFuture = null; } } } @@ -307,18 +302,12 @@ private void scheduleDeadlineTimeout() { final @Nullable Long deadlineTimeOut = transactionOptions.getDeadlineTimeout(); if (deadlineTimeOut != null) { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (timer != null) { + if (timerExecutorService != null) { cancelDeadlineTimer(); isDeadlineTimerRunning.set(true); - deadlineTimeoutTask = - new TimerTask() { - @Override - public void run() { - onDeadlineTimeoutReached(); - } - }; try { - timer.schedule(deadlineTimeoutTask, deadlineTimeOut); + deadlineTimeoutFuture = + timerExecutorService.schedule(this::onDeadlineTimeoutReached, deadlineTimeOut); } catch (Throwable e) { scopes .getOptions() @@ -335,10 +324,10 @@ public void run() { private void cancelDeadlineTimer() { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (deadlineTimeoutTask != null) { - deadlineTimeoutTask.cancel(); + if (deadlineTimeoutFuture != null) { + deadlineTimeoutFuture.cancel(false); isDeadlineTimerRunning.set(false); - deadlineTimeoutTask = null; + deadlineTimeoutFuture = null; } } } @@ -973,20 +962,20 @@ Span getRoot() { @TestOnly @Nullable - TimerTask getIdleTimeoutTask() { - return idleTimeoutTask; + Future getIdleTimeoutFuture() { + return idleTimeoutFuture; } @TestOnly @Nullable - TimerTask getDeadlineTimeoutTask() { - return deadlineTimeoutTask; + Future getDeadlineTimeoutFuture() { + return deadlineTimeoutFuture; } @TestOnly @Nullable - Timer getTimer() { - return timer; + ISentryExecutorService getTimerExecutorService() { + return timerExecutorService; } @TestOnly diff --git a/sentry/src/test/java/io/sentry/ScopesTest.kt b/sentry/src/test/java/io/sentry/ScopesTest.kt index a4c2c76845..cd5e79e159 100644 --- a/sentry/src/test/java/io/sentry/ScopesTest.kt +++ b/sentry/src/test/java/io/sentry/ScopesTest.kt @@ -1944,6 +1944,32 @@ class ScopesTest { verify(executor).close(any()) } + @Test + fun `Scopes with isRestarting true should not close the timer executor`() { + val timerExecutor = mock() + val options = + SentryOptions().apply { + dsn = "https://key@sentry.io/proj" + setTimerExecutorService(timerExecutor) + } + val sut = createScopes(options) + sut.close(true) + verify(timerExecutor, never()).close(any()) + } + + @Test + fun `Scopes with isRestarting false should close the timer executor`() { + val timerExecutor = mock() + val options = + SentryOptions().apply { + dsn = "https://key@sentry.io/proj" + setTimerExecutorService(timerExecutor) + } + val sut = createScopes(options) + sut.close(false) + verify(timerExecutor).close(any()) + } + @Test fun `Scopes close should clear the scope`() { val options = SentryOptions().apply { dsn = "https://key@sentry.io/proj" } diff --git a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt index 2ccba650ad..8d8b77ee0c 100644 --- a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt +++ b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt @@ -94,6 +94,22 @@ class SentryExecutorServiceTest { sentryExecutor.close(15000) } + @Test + fun `SentryExecutorService enables removeOnCancelPolicy when requested`() { + val sentryExecutor = SentryExecutorService(null, true) + val executor = sentryExecutor.getProperty("executorService") + assertTrue(executor.removeOnCancelPolicy) + sentryExecutor.close(15000) + } + + @Test + fun `SentryExecutorService does not enable removeOnCancelPolicy by default`() { + val sentryExecutor = SentryExecutorService(null) + val executor = sentryExecutor.getProperty("executorService") + assertFalse(executor.removeOnCancelPolicy) + sentryExecutor.close(15000) + } + @Test fun `SentryExecutorService isClosed returns true if executor is shutdown`() { val executor = mock() diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 3b808dd222..e8b8fab091 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -13,6 +13,7 @@ import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNotEquals import kotlin.test.assertNotNull +import kotlin.test.assertNotSame import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue @@ -913,7 +914,7 @@ class SentryTracerTest { @Test fun `when initialized without deadlineTimeout, does not schedule finish timer`() { val transaction = fixture.getSut() - assertNull(transaction.deadlineTimeoutTask) + assertNull(transaction.deadlineTimeoutFuture) } @Test @@ -921,7 +922,7 @@ class SentryTracerTest { val transaction = fixture.getSut(deadlineTimeout = 50) assertTrue(transaction.isDeadlineTimerRunning.get()) - assertNotNull(transaction.deadlineTimeoutTask) + assertNotNull(transaction.deadlineTimeoutFuture) } @Test @@ -949,7 +950,7 @@ class SentryTracerTest { transaction.finish(SpanStatus.OK) assertEquals(transaction.isDeadlineTimerRunning.get(), false) - assertNull(transaction.deadlineTimeoutTask) + assertNull(transaction.deadlineTimeoutFuture) assertEquals(transaction.isFinished, true) assertEquals(SpanStatus.OK, transaction.status) assertEquals(SpanStatus.OK, span.status) @@ -958,26 +959,26 @@ class SentryTracerTest { @Test fun `when initialized with idleTimeout it has no influence on deadline timeout`() { val transaction = fixture.getSut(idleTimeout = 3000, deadlineTimeout = 20) - val deadlineTimeoutTask = transaction.deadlineTimeoutTask + val deadlineTimeoutFuture = transaction.deadlineTimeoutFuture val span = transaction.startChild("op") // when the span finishes, it re-schedules the idle task span.finish() // but the deadline timeout task should not be re-scheduled - assertEquals(deadlineTimeoutTask, transaction.deadlineTimeoutTask) + assertSame(deadlineTimeoutFuture, transaction.deadlineTimeoutFuture) } @Test fun `when initialized without idleTimeout, does not schedule finish timer`() { val transaction = fixture.getSut() - assertNull(transaction.idleTimeoutTask) + assertNull(transaction.idleTimeoutFuture) } @Test fun `when initialized with idleTimeout, schedules finish timer`() { val transaction = fixture.getSut(idleTimeout = 50) - assertNotNull(transaction.idleTimeoutTask) + assertNotNull(transaction.idleTimeoutFuture) } @Test @@ -1008,22 +1009,23 @@ class SentryTracerTest { transaction.startChild("op") - assertNull(transaction.idleTimeoutTask) + assertNull(transaction.idleTimeoutFuture) } @Test fun `when a child is finished and the transaction is idle, resets the timer`() { val transaction = fixture.getSut(waitForChildren = true, idleTimeout = 3000) - val initialTime = transaction.idleTimeoutTask!!.scheduledExecutionTime() + val initialFuture = transaction.idleTimeoutFuture val span = transaction.startChild("op") - Thread.sleep(1) span.finish() - val timerAfterFinishingChild = transaction.idleTimeoutTask!!.scheduledExecutionTime() + // finishing the child re-schedules the idle timeout, replacing the pending future + val futureAfterFinishingChild = transaction.idleTimeoutFuture - assertTrue { timerAfterFinishingChild > initialTime } + assertNotNull(futureAfterFinishingChild) + assertNotSame(initialFuture, futureAfterFinishingChild) } @Test @@ -1035,7 +1037,7 @@ class SentryTracerTest { Thread.sleep(1) span.finish() - assertNull(transaction.idleTimeoutTask) + assertNull(transaction.idleTimeoutFuture) } @Test @@ -1080,7 +1082,7 @@ class SentryTracerTest { trimEnd = true, samplingDecision = TracesSamplingDecision(true), ) - assertNotNull(transaction.timer) + assertNotNull(transaction.timerExecutorService) } @Test @@ -1092,7 +1094,7 @@ class SentryTracerTest { trimEnd = true, samplingDecision = TracesSamplingDecision(true), ) - assertNull(transaction.timer) + assertNull(transaction.timerExecutorService) } @Test @@ -1104,9 +1106,9 @@ class SentryTracerTest { trimEnd = true, samplingDecision = TracesSamplingDecision(true), ) - assertNotNull(transaction.timer) + assertNotNull(transaction.timerExecutorService) transaction.finish(SpanStatus.OK) - assertNull(transaction.timer) + assertNull(transaction.timerExecutorService) } @Test @@ -1539,18 +1541,18 @@ class SentryTracerTest { } @Test - fun `when timer is cancelled, schedule finish does not crash`() { + fun `when timer executor is shut down, schedule finish does not crash`() { val tracer = fixture.getSut(idleTimeout = 50, deadlineTimeout = 100) - tracer.timer!!.cancel() + fixture.options.timerExecutorService.close(0) tracer.scheduleFinish() } @Test - fun `when timer is cancelled, schedule finish finishes the transaction immediately`() { + fun `when timer executor is shut down, schedule finish finishes the transaction immediately`() { val tracer = fixture.getSut(idleTimeout = 50) tracer.startChild("load").finish() - tracer.timer!!.cancel() + fixture.options.timerExecutorService.close(0) tracer.scheduleFinish() assertTrue(tracer.isFinished)