Skip to content

Commit ba5f1d5

Browse files
runningcodeclaude0xadam-brown
authored
fix(core): Report no-op executor tasks as cancelled (#5874)
* fix(core): Report no-op executor tasks as cancelled (JAVA-628) NoOpSentryExecutorService returned a FutureTask that is never run and never cancelled, so a caller had no way to tell a dropped task from a queued one: isCancelled() and isDone() both stay false forever, and get() blocks until it times out. Return an already-cancelled Future instead, matching what SentryExecutorService already does when its work queue is full, and state the requirement on ISentryExecutorService so future implementations honour it. CancelledFuture moves out of SentryExecutorService so both services share one implementation. An ActivityLifecycleIntegration test asserted a freshly scheduled ttfd timeout was not cancelled while running on the no-op executor, so it was really pinning the old pending-forever behaviour rather than the scheduling it names. It now uses DeferredExecutorService, which queues. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * changelog * Update sentry/src/main/java/io/sentry/ISentryExecutorService.java Co-authored-by: arb <adam.brown@sentry.io> * Apply suggestions from code review Co-authored-by: arb <adam.brown@sentry.io> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: arb <adam.brown@sentry.io>
1 parent 6de3962 commit ba5f1d5

8 files changed

Lines changed: 85 additions & 39 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
- Prevent inflated cold app start when the OS spawns the process in the background (e.g. FCM push) on API 35+ ([#5841](https://github.com/getsentry/sentry-java/pull/5841))
88
- Avoid a CPU busy-loop when recording discarded log or metric envelopes under rate limiting ([#5835](https://github.com/getsentry/sentry-java/pull/5835))
99
- `ClientReportRecorder` now reads the item count from the envelope item header instead of deserializing the payload, which under sustained rate limiting could pin CPU cores while repeatedly throwing exceptions
10+
- Report tasks handed to a no-op `ISentryExecutorService` as cancelled ([#5874](https://github.com/getsentry/sentry-java/pull/5874))
11+
- `NoOpSentryExecutorService` previously returned a `Future` that was never run and never cancelled, so callers could not tell a dropped task from a queued one and `get()` would block until its timeout
1012

1113
### Performance
1214

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1975,6 +1975,8 @@ class ActivityLifecycleIntegrationTest {
19751975
val sut = fixture.getSut()
19761976
fixture.options.tracesSampleRate = 1.0
19771977
fixture.options.isEnableTimeToFullDisplayTracing = true
1978+
// the timeout has to be really scheduled for cancelling it to be observable
1979+
fixture.options.executorService = DeferredExecutorService()
19781980
sut.register(fixture.scopes, fixture.options)
19791981
val activity = mock<Activity>()
19801982
val activity2 = mock<Activity>()

‎sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,15 @@ class DeferredExecutorService : ISentryExecutorService {
7676
}
7777

7878
class NonOverridableNoOpSentryExecutorService : ISentryExecutorService {
79-
override fun submit(runnable: Runnable): Future<*> = FutureTask<Void> { null }
79+
// mirrors NoOpSentryExecutorService: a task that is never run is reported as cancelled, so
80+
// callers can tell it apart from a queued one
81+
private fun <T> cancelledFuture(): FutureTask<T> = FutureTask<T> { null }.apply { cancel(false) }
8082

81-
override fun <T> submit(callable: Callable<T>): Future<T> = FutureTask<T> { null }
83+
override fun submit(runnable: Runnable): Future<*> = cancelledFuture<Void>()
8284

83-
override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> =
84-
FutureTask<Void> { null }
85+
override fun <T> submit(callable: Callable<T>): Future<T> = cancelledFuture()
86+
87+
override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> = cancelledFuture<Void>()
8588

8689
override fun close(timeoutMillis: Long) {}
8790

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package io.sentry;
2+
3+
import java.util.concurrent.CancellationException;
4+
import java.util.concurrent.Future;
5+
import java.util.concurrent.TimeUnit;
6+
import org.jetbrains.annotations.NotNull;
7+
8+
/**
9+
* The {@link Future} an {@link ISentryExecutorService} hands back for a task it will never run.
10+
* Callers can tell such a task apart from an accepted one through {@link #isCancelled()}, and are
11+
* spared blocking in {@link #get()} on a result that is never coming.
12+
*/
13+
final class CancelledFuture<T> implements Future<T> {
14+
@Override
15+
public boolean cancel(final boolean mayInterruptIfRunning) {
16+
return true;
17+
}
18+
19+
@Override
20+
public boolean isCancelled() {
21+
return true;
22+
}
23+
24+
@Override
25+
public boolean isDone() {
26+
return true;
27+
}
28+
29+
@Override
30+
public T get() {
31+
throw new CancellationException();
32+
}
33+
34+
@Override
35+
public T get(final long timeout, final @NotNull TimeUnit unit) {
36+
throw new CancellationException();
37+
}
38+
}

‎sentry/src/main/java/io/sentry/ISentryExecutorService.java‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,23 @@
66
import org.jetbrains.annotations.ApiStatus;
77
import org.jetbrains.annotations.NotNull;
88

9-
/** Sentry Executor Service that sends cached events and envelopes on App. start. */
9+
/**
10+
* Sentry Executor Service that sends cached events and envelopes on App start.
11+
*
12+
* <p>Implementations that drop a task instead of running it — a no-op service, a full work queue —
13+
* must report that through the returned {@link Future}: either throw {@link
14+
* RejectedExecutionException} or return a {@link CancelledFuture}. Callers rely on this to tell a
15+
* queued task from a dropped one, so a Future that will never complete and never reports
16+
* cancellation leaves them waiting on a task that is never coming.
17+
*/
1018
@ApiStatus.Internal
1119
public interface ISentryExecutorService {
1220

1321
/**
1422
* Submits a Runnable to the ThreadExecutor
1523
*
1624
* @param runnable the Runnable
17-
* @return a Future of the Runnable
25+
* @return a Future of the Runnable, already cancelled if the task will not be run
1826
*/
1927
@NotNull
2028
Future<?> submit(final @NotNull Runnable runnable) throws RejectedExecutionException;
@@ -23,11 +31,18 @@ public interface ISentryExecutorService {
2331
* Submits a Callable to the ThreadExecutor
2432
*
2533
* @param callable the Callable
26-
* @return a Future of the Callable
34+
* @return a Future of the Callable, already cancelled if the task will not be run
2735
*/
2836
@NotNull
2937
<T> Future<T> submit(final @NotNull Callable<T> callable) throws RejectedExecutionException;
3038

39+
/**
40+
* Schedules a Runnable on the ThreadExecutor
41+
*
42+
* @param runnable the Runnable
43+
* @param delayMillis how long to wait before running it
44+
* @return a Future of the Runnable, already cancelled if the task will not be run
45+
*/
3146
@NotNull
3247
Future<?> schedule(final @NotNull Runnable runnable, final long delayMillis)
3348
throws RejectedExecutionException;

‎sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import java.util.concurrent.Callable;
44
import java.util.concurrent.Future;
5-
import java.util.concurrent.FutureTask;
65
import org.jetbrains.annotations.ApiStatus;
76
import org.jetbrains.annotations.NotNull;
87

@@ -18,17 +17,17 @@ private NoOpSentryExecutorService() {}
1817

1918
@Override
2019
public @NotNull Future<?> submit(final @NotNull Runnable runnable) {
21-
return new FutureTask<>(() -> null);
20+
return new CancelledFuture<>();
2221
}
2322

2423
@Override
2524
public @NotNull <T> Future<T> submit(final @NotNull Callable<T> callable) {
26-
return new FutureTask<>(() -> null);
25+
return new CancelledFuture<>();
2726
}
2827

2928
@Override
3029
public @NotNull Future<?> schedule(@NotNull Runnable runnable, long delayMillis) {
31-
return new FutureTask<>(() -> null);
30+
return new CancelledFuture<>();
3231
}
3332

3433
@Override

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

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import io.sentry.util.AutoClosableReentrantLock;
44
import java.util.concurrent.Callable;
5-
import java.util.concurrent.CancellationException;
65
import java.util.concurrent.Future;
76
import java.util.concurrent.RejectedExecutionException;
87
import java.util.concurrent.ScheduledThreadPoolExecutor;
@@ -155,31 +154,4 @@ private static final class SentryExecutorServiceThread extends Thread {
155154
super(r, name);
156155
}
157156
}
158-
159-
private static final class CancelledFuture<T> implements Future<T> {
160-
@Override
161-
public boolean cancel(final boolean mayInterruptIfRunning) {
162-
return true;
163-
}
164-
165-
@Override
166-
public boolean isCancelled() {
167-
return true;
168-
}
169-
170-
@Override
171-
public boolean isDone() {
172-
return true;
173-
}
174-
175-
@Override
176-
public T get() {
177-
throw new CancellationException();
178-
}
179-
180-
@Override
181-
public T get(final long timeout, final @NotNull TimeUnit unit) {
182-
throw new CancellationException();
183-
}
184-
}
185157
}

‎sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package io.sentry
22

3+
import com.google.common.truth.Truth.assertThat
34
import java.util.concurrent.Callable
5+
import java.util.concurrent.CancellationException
46
import kotlin.test.Test
7+
import kotlin.test.assertFailsWith
58
import kotlin.test.assertFalse
69
import kotlin.test.assertNotNull
710
import org.mockito.kotlin.mock
@@ -27,6 +30,18 @@ class NoOpSentryExecutorServiceTest {
2730
assertNotNull(future)
2831
}
2932

33+
@Test
34+
fun `submitted tasks are reported as cancelled instead of pending forever`() {
35+
assertThat(sut.submit(mock<Runnable>()).isCancelled).isTrue()
36+
assertThat(sut.submit(mock<Callable<*>>()).isCancelled).isTrue()
37+
assertThat(sut.schedule(mock(), 0).isCancelled).isTrue()
38+
}
39+
40+
@Test
41+
fun `getting the result of a dropped task fails fast`() {
42+
assertFailsWith<CancellationException> { sut.submit(mock<Runnable>()).get() }
43+
}
44+
3045
@Test fun `close does not throw`() = sut.close(0)
3146

3247
@Test

0 commit comments

Comments
 (0)