diff --git a/CHANGELOG.md b/CHANGELOG.md index af29b22ccb5..17642fb819f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,10 @@ - Prevent logs and metrics from remaining queued after a flush scheduling race ([#5756](https://github.com/getsentry/sentry-java/pull/5756)) - Fix main thread identification for tombstone (native crash) events ([#5742](https://github.com/getsentry/sentry-java/pull/5742)) +### 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)) + ### Dependencies - Bump OpenTelemetry to support Spring Boot 4.1 ([#5573](https://github.com/getsentry/sentry-java/pull/5573)) 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 1d8fa06f3bb..166129f601b 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 @@ -919,6 +920,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 00183bc9b30..e0bf144178f 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3719,6 +3719,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; @@ -3884,6 +3885,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 3b67b94916e..936a331e3d4 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 275e2a3c614..8bba9d92e4f 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -353,6 +353,12 @@ private static void init(final @NotNull SentryOptions options, final boolean glo options.setExecutorService(new SentryExecutorService(options)); } + if (options.getTimerExecutorService().isClosed()) { + options.setTimerExecutorService( + new SentryExecutorService( + options, true, SentryExecutorService.TIMER_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS)); + } + // 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 1936bb9c360..a469dca5853 100644 --- a/sentry/src/main/java/io/sentry/SentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/SentryExecutorService.java @@ -22,6 +22,12 @@ public final class SentryExecutorService implements ISentryExecutorService { */ private static final int MAX_QUEUE_SIZE = 271; + /** + * How long the timer executor's worker thread stays alive while idle before self-terminating, so + * an instance abandoned on SDK restart doesn't leak a live thread once its queue drains. + */ + static final long TIMER_KEEP_ALIVE_SECONDS = 30; + private final @NotNull ScheduledThreadPoolExecutor executorService; private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); @@ -39,6 +45,19 @@ public SentryExecutorService(final @Nullable SentryOptions options) { this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), options); } + SentryExecutorService( + final @Nullable SentryOptions options, + final boolean removeOnCancelPolicy, + final long keepAliveTime, + final @NotNull TimeUnit keepAliveTimeUnit) { + 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); + executorService.setKeepAliveTime(keepAliveTime, keepAliveTimeUnit); + 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 3c55f5e1cfa..cde0c37ba90 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -44,6 +44,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CopyOnWriteArraySet; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import javax.net.ssl.SSLSocketFactory; import org.jetbrains.annotations.ApiStatus; @@ -317,6 +318,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. @@ -683,6 +692,15 @@ public void activate() { executorService = new SentryExecutorService(this); } + 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, SentryExecutorService.TIMER_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS); + } + // 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. @@ -1570,6 +1588,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 9729ac406b1..723538b9924 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,12 @@ 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; + // Whether timeout tasks may still be scheduled. Set to false once the tracer is finished. The + // executor itself is owned by the options (shared SDK-wide) and obtained from there when needed. + private volatile boolean timersEnabled = false; private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock(); private final @NotNull AutoClosableReentrantLock tracerLock = new AutoClosableReentrantLock(); @@ -99,7 +100,7 @@ public SentryTracer( if (transactionOptions.getIdleTimeout() != null || transactionOptions.getDeadlineTimeout() != null) { - timer = new Timer(true); + timersEnabled = true; scheduleDeadlineTimeout(); scheduleFinish(); @@ -109,22 +110,19 @@ public SentryTracer( @Override public void scheduleFinish() { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (timer != null) { + if (timersEnabled) { 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 = + scopes + .getOptions() + .getTimerExecutorService() + .schedule(this::onIdleTimeoutReached, idleTimeout); } catch (Throwable e) { scopes .getOptions() @@ -265,13 +263,12 @@ public void finish( }); final SentryTransaction transaction = new SentryTransaction(this); - if (timer != null) { + if (timersEnabled) { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (timer != null) { + if (timersEnabled) { cancelIdleTimer(); cancelDeadlineTimer(); - timer.cancel(); - timer = null; + timersEnabled = false; } } } @@ -295,10 +292,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 +304,15 @@ private void scheduleDeadlineTimeout() { final @Nullable Long deadlineTimeOut = transactionOptions.getDeadlineTimeout(); if (deadlineTimeOut != null) { try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) { - if (timer != null) { + if (timersEnabled) { cancelDeadlineTimer(); isDeadlineTimerRunning.set(true); - deadlineTimeoutTask = - new TimerTask() { - @Override - public void run() { - onDeadlineTimeoutReached(); - } - }; try { - timer.schedule(deadlineTimeoutTask, deadlineTimeOut); + deadlineTimeoutFuture = + scopes + .getOptions() + .getTimerExecutorService() + .schedule(this::onDeadlineTimeoutReached, deadlineTimeOut); } catch (Throwable e) { scopes .getOptions() @@ -335,10 +329,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 +967,19 @@ 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; + boolean areTimersEnabled() { + return timersEnabled; } @TestOnly diff --git a/sentry/src/test/java/io/sentry/ScopesTest.kt b/sentry/src/test/java/io/sentry/ScopesTest.kt index 4b9b3095d53..9d598aec885 100644 --- a/sentry/src/test/java/io/sentry/ScopesTest.kt +++ b/sentry/src/test/java/io/sentry/ScopesTest.kt @@ -1946,6 +1946,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 57dfb578ee9..153feecb4a4 100644 --- a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt +++ b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt @@ -1,5 +1,6 @@ package io.sentry +import io.sentry.test.getProperty import java.util.concurrent.BlockingQueue import java.util.concurrent.Callable import java.util.concurrent.CancellationException @@ -93,6 +94,22 @@ class SentryExecutorServiceTest { sentryExecutor.close(15000) } + @Test + fun `SentryExecutorService enables removeOnCancelPolicy when requested`() { + val sentryExecutor = SentryExecutorService(null, true, 30, TimeUnit.SECONDS) + 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 3b808dd2220..20eeafffe92 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) + assertTrue(transaction.areTimersEnabled()) } @Test @@ -1092,7 +1094,7 @@ class SentryTracerTest { trimEnd = true, samplingDecision = TracesSamplingDecision(true), ) - assertNull(transaction.timer) + assertFalse(transaction.areTimersEnabled()) } @Test @@ -1104,9 +1106,9 @@ class SentryTracerTest { trimEnd = true, samplingDecision = TracesSamplingDecision(true), ) - assertNotNull(transaction.timer) + assertTrue(transaction.areTimersEnabled()) transaction.finish(SpanStatus.OK) - assertNull(transaction.timer) + assertFalse(transaction.areTimersEnabled()) } @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)