Skip to content

Commit 57da81e

Browse files
runningcodeclaude
andcommitted
refactor(core): Look up shared timer executor on demand (JAVA-570)
SentryTracer cached the shared timer executor in a field for the lifetime of the transaction. Replace that field with a boolean flag tracking whether timeouts may still be scheduled, and fetch the executor from the options each time one is scheduled. This ensures the tracer always uses the executor currently held by the options (e.g. the fresh one installed after an SDK restart) rather than a stale reference. Also make the timer executor's keep-alive duration a constructor argument backed by the named TIMER_KEEP_ALIVE_SECONDS constant, and raise it from 10s to 30s so the shared worker thread is less likely to be torn down and respawned between transactions under normal use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 9a94c51 commit 57da81e

6 files changed

Lines changed: 44 additions & 26 deletions

File tree

sentry/src/main/java/io/sentry/Sentry.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,9 @@ private static void init(final @NotNull SentryOptions options, final boolean glo
354354
}
355355

356356
if (options.getTimerExecutorService().isClosed()) {
357-
options.setTimerExecutorService(new SentryExecutorService(options, true));
357+
options.setTimerExecutorService(
358+
new SentryExecutorService(
359+
options, true, SentryExecutorService.TIMER_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS));
358360
}
359361

360362
// load lazy fields of the options in a separate thread

sentry/src/main/java/io/sentry/SentryExecutorService.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ public final class SentryExecutorService implements ISentryExecutorService {
2222
*/
2323
private static final int MAX_QUEUE_SIZE = 271;
2424

25+
/**
26+
* How long the timer executor's worker thread stays alive while idle before self-terminating, so
27+
* an instance abandoned on SDK restart doesn't leak a live thread once its queue drains.
28+
*/
29+
static final long TIMER_KEEP_ALIVE_SECONDS = 30;
30+
2531
private final @NotNull ScheduledThreadPoolExecutor executorService;
2632
private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
2733

@@ -39,14 +45,16 @@ public SentryExecutorService(final @Nullable SentryOptions options) {
3945
this(new ScheduledThreadPoolExecutor(1, new SentryExecutorServiceThreadFactory()), options);
4046
}
4147

42-
SentryExecutorService(final @Nullable SentryOptions options, final boolean removeOnCancelPolicy) {
48+
SentryExecutorService(
49+
final @Nullable SentryOptions options,
50+
final boolean removeOnCancelPolicy,
51+
final long keepAliveTime,
52+
final @NotNull TimeUnit keepAliveTimeUnit) {
4353
this(options);
4454
// removes cancelled tasks from the work queue immediately instead of leaving them until their
4555
// scheduled time; useful for executors that frequently reschedule (e.g. transaction timeouts)
4656
executorService.setRemoveOnCancelPolicy(removeOnCancelPolicy);
47-
// let the worker thread die when idle so an executor abandoned on SDK restart (its pending
48-
// timeouts still fire) doesn't leak a live thread once its queue drains
49-
executorService.setKeepAliveTime(10, TimeUnit.SECONDS);
57+
executorService.setKeepAliveTime(keepAliveTime, keepAliveTimeUnit);
5058
executorService.allowCoreThreadTimeOut(true);
5159
}
5260

sentry/src/main/java/io/sentry/SentryOptions.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import java.util.concurrent.ConcurrentHashMap;
4545
import java.util.concurrent.CopyOnWriteArrayList;
4646
import java.util.concurrent.CopyOnWriteArraySet;
47+
import java.util.concurrent.TimeUnit;
4748
import java.util.concurrent.atomic.AtomicBoolean;
4849
import javax.net.ssl.SSLSocketFactory;
4950
import org.jetbrains.annotations.ApiStatus;
@@ -695,7 +696,9 @@ public void activate() {
695696
// Not prewarmed: its single worker thread is spawned lazily on the first scheduled timeout
696697
// and then reused across all transactions. removeOnCancelPolicy keeps the work queue from
697698
// accumulating cancelled timeouts (idle timers are cancelled and rescheduled per child span).
698-
timerExecutorService = new SentryExecutorService(this, true);
699+
timerExecutorService =
700+
new SentryExecutorService(
701+
this, true, SentryExecutorService.TIMER_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS);
699702
}
700703

701704
// SpotlightIntegration is loaded via reflection to allow the sentry-spotlight module

sentry/src/main/java/io/sentry/SentryTracer.java

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,9 @@ public final class SentryTracer implements ITransaction {
3939
private volatile @Nullable Future<?> idleTimeoutFuture;
4040
private volatile @Nullable Future<?> deadlineTimeoutFuture;
4141

42-
// Shared executor used to schedule the timeout tasks. Null once the tracer is finished, at which
43-
// point no more timeouts may be scheduled. It is never shut down here since it is shared
44-
// SDK-wide.
45-
private volatile @Nullable ISentryExecutorService timerExecutorService = null;
42+
// Whether timeout tasks may still be scheduled. Set to false once the tracer is finished. The
43+
// executor itself is owned by the options (shared SDK-wide) and obtained from there when needed.
44+
private volatile boolean timersEnabled = false;
4645
private final @NotNull AutoClosableReentrantLock timerLock = new AutoClosableReentrantLock();
4746
private final @NotNull AutoClosableReentrantLock tracerLock = new AutoClosableReentrantLock();
4847

@@ -101,7 +100,7 @@ public SentryTracer(
101100

102101
if (transactionOptions.getIdleTimeout() != null
103102
|| transactionOptions.getDeadlineTimeout() != null) {
104-
timerExecutorService = scopes.getOptions().getTimerExecutorService();
103+
timersEnabled = true;
105104

106105
scheduleDeadlineTimeout();
107106
scheduleFinish();
@@ -111,7 +110,7 @@ public SentryTracer(
111110
@Override
112111
public void scheduleFinish() {
113112
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
114-
if (timerExecutorService != null) {
113+
if (timersEnabled) {
115114
final @Nullable Long idleTimeout = transactionOptions.getIdleTimeout();
116115

117116
if (idleTimeout != null) {
@@ -120,7 +119,10 @@ public void scheduleFinish() {
120119

121120
try {
122121
idleTimeoutFuture =
123-
timerExecutorService.schedule(this::onIdleTimeoutReached, idleTimeout);
122+
scopes
123+
.getOptions()
124+
.getTimerExecutorService()
125+
.schedule(this::onIdleTimeoutReached, idleTimeout);
124126
} catch (Throwable e) {
125127
scopes
126128
.getOptions()
@@ -261,12 +263,12 @@ public void finish(
261263
});
262264
final SentryTransaction transaction = new SentryTransaction(this);
263265

264-
if (timerExecutorService != null) {
266+
if (timersEnabled) {
265267
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
266-
if (timerExecutorService != null) {
268+
if (timersEnabled) {
267269
cancelIdleTimer();
268270
cancelDeadlineTimer();
269-
timerExecutorService = null;
271+
timersEnabled = false;
270272
}
271273
}
272274
}
@@ -302,12 +304,15 @@ private void scheduleDeadlineTimeout() {
302304
final @Nullable Long deadlineTimeOut = transactionOptions.getDeadlineTimeout();
303305
if (deadlineTimeOut != null) {
304306
try (final @NotNull ISentryLifecycleToken ignored = timerLock.acquire()) {
305-
if (timerExecutorService != null) {
307+
if (timersEnabled) {
306308
cancelDeadlineTimer();
307309
isDeadlineTimerRunning.set(true);
308310
try {
309311
deadlineTimeoutFuture =
310-
timerExecutorService.schedule(this::onDeadlineTimeoutReached, deadlineTimeOut);
312+
scopes
313+
.getOptions()
314+
.getTimerExecutorService()
315+
.schedule(this::onDeadlineTimeoutReached, deadlineTimeOut);
311316
} catch (Throwable e) {
312317
scopes
313318
.getOptions()
@@ -973,9 +978,8 @@ Future<?> getDeadlineTimeoutFuture() {
973978
}
974979

975980
@TestOnly
976-
@Nullable
977-
ISentryExecutorService getTimerExecutorService() {
978-
return timerExecutorService;
981+
boolean areTimersEnabled() {
982+
return timersEnabled;
979983
}
980984

981985
@TestOnly

sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.sentry
22

3+
import io.sentry.test.getProperty
34
import java.util.concurrent.BlockingQueue
45
import java.util.concurrent.Callable
56
import java.util.concurrent.CancellationException
@@ -95,7 +96,7 @@ class SentryExecutorServiceTest {
9596

9697
@Test
9798
fun `SentryExecutorService enables removeOnCancelPolicy when requested`() {
98-
val sentryExecutor = SentryExecutorService(null, true)
99+
val sentryExecutor = SentryExecutorService(null, true, 30, TimeUnit.SECONDS)
99100
val executor = sentryExecutor.getProperty<ScheduledThreadPoolExecutor>("executorService")
100101
assertTrue(executor.removeOnCancelPolicy)
101102
sentryExecutor.close(15000)

sentry/src/test/java/io/sentry/SentryTracerTest.kt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1082,7 +1082,7 @@ class SentryTracerTest {
10821082
trimEnd = true,
10831083
samplingDecision = TracesSamplingDecision(true),
10841084
)
1085-
assertNotNull(transaction.timerExecutorService)
1085+
assertTrue(transaction.areTimersEnabled())
10861086
}
10871087

10881088
@Test
@@ -1094,7 +1094,7 @@ class SentryTracerTest {
10941094
trimEnd = true,
10951095
samplingDecision = TracesSamplingDecision(true),
10961096
)
1097-
assertNull(transaction.timerExecutorService)
1097+
assertFalse(transaction.areTimersEnabled())
10981098
}
10991099

11001100
@Test
@@ -1106,9 +1106,9 @@ class SentryTracerTest {
11061106
trimEnd = true,
11071107
samplingDecision = TracesSamplingDecision(true),
11081108
)
1109-
assertNotNull(transaction.timerExecutorService)
1109+
assertTrue(transaction.areTimersEnabled())
11101110
transaction.finish(SpanStatus.OK)
1111-
assertNull(transaction.timerExecutorService)
1111+
assertFalse(transaction.areTimersEnabled())
11121112
}
11131113

11141114
@Test

0 commit comments

Comments
 (0)