diff --git a/CHANGELOG.md b/CHANGELOG.md index 89d412bcfdd..226c7cfb54f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ - Avoid a CPU busy-loop when recording discarded log or metric envelopes under rate limiting ([#5835](https://github.com/getsentry/sentry-java/pull/5835)) - `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 +- Report tasks handed to a no-op `ISentryExecutorService` as cancelled ([#5874](https://github.com/getsentry/sentry-java/pull/5874)) + - `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 ### Performance 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 166129f601b..c79d418efe5 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 @@ -1975,6 +1975,8 @@ class ActivityLifecycleIntegrationTest { val sut = fixture.getSut() fixture.options.tracesSampleRate = 1.0 fixture.options.isEnableTimeToFullDisplayTracing = true + // the timeout has to be really scheduled for cancelling it to be observable + fixture.options.executorService = DeferredExecutorService() sut.register(fixture.scopes, fixture.options) val activity = mock() val activity2 = mock() diff --git a/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt b/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt index 79076e9dc8f..b66c0f29475 100644 --- a/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt +++ b/sentry-test-support/src/main/kotlin/io/sentry/test/Mocks.kt @@ -76,12 +76,15 @@ class DeferredExecutorService : ISentryExecutorService { } class NonOverridableNoOpSentryExecutorService : ISentryExecutorService { - override fun submit(runnable: Runnable): Future<*> = FutureTask { null } + // mirrors NoOpSentryExecutorService: a task that is never run is reported as cancelled, so + // callers can tell it apart from a queued one + private fun cancelledFuture(): FutureTask = FutureTask { null }.apply { cancel(false) } - override fun submit(callable: Callable): Future = FutureTask { null } + override fun submit(runnable: Runnable): Future<*> = cancelledFuture() - override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> = - FutureTask { null } + override fun submit(callable: Callable): Future = cancelledFuture() + + override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> = cancelledFuture() override fun close(timeoutMillis: Long) {} diff --git a/sentry/src/main/java/io/sentry/CancelledFuture.java b/sentry/src/main/java/io/sentry/CancelledFuture.java new file mode 100644 index 00000000000..502651597cb --- /dev/null +++ b/sentry/src/main/java/io/sentry/CancelledFuture.java @@ -0,0 +1,38 @@ +package io.sentry; + +import java.util.concurrent.CancellationException; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.jetbrains.annotations.NotNull; + +/** + * The {@link Future} an {@link ISentryExecutorService} hands back for a task it will never run. + * Callers can tell such a task apart from an accepted one through {@link #isCancelled()}, and are + * spared blocking in {@link #get()} on a result that is never coming. + */ +final class CancelledFuture implements Future { + @Override + public boolean cancel(final boolean mayInterruptIfRunning) { + return true; + } + + @Override + public boolean isCancelled() { + return true; + } + + @Override + public boolean isDone() { + return true; + } + + @Override + public T get() { + throw new CancellationException(); + } + + @Override + public T get(final long timeout, final @NotNull TimeUnit unit) { + throw new CancellationException(); + } +} diff --git a/sentry/src/main/java/io/sentry/ISentryExecutorService.java b/sentry/src/main/java/io/sentry/ISentryExecutorService.java index 9bdef8db2b7..656308dae4a 100644 --- a/sentry/src/main/java/io/sentry/ISentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/ISentryExecutorService.java @@ -6,7 +6,15 @@ import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; -/** Sentry Executor Service that sends cached events and envelopes on App. start. */ +/** + * Sentry Executor Service that sends cached events and envelopes on App start. + * + *

Implementations that drop a task instead of running it — a no-op service, a full work queue — + * must report that through the returned {@link Future}: either throw {@link + * RejectedExecutionException} or return a {@link CancelledFuture}. Callers rely on this to tell a + * queued task from a dropped one, so a Future that will never complete and never reports + * cancellation leaves them waiting on a task that is never coming. + */ @ApiStatus.Internal public interface ISentryExecutorService { @@ -14,7 +22,7 @@ public interface ISentryExecutorService { * Submits a Runnable to the ThreadExecutor * * @param runnable the Runnable - * @return a Future of the Runnable + * @return a Future of the Runnable, already cancelled if the task will not be run */ @NotNull Future submit(final @NotNull Runnable runnable) throws RejectedExecutionException; @@ -23,11 +31,18 @@ public interface ISentryExecutorService { * Submits a Callable to the ThreadExecutor * * @param callable the Callable - * @return a Future of the Callable + * @return a Future of the Callable, already cancelled if the task will not be run */ @NotNull Future submit(final @NotNull Callable callable) throws RejectedExecutionException; + /** + * Schedules a Runnable on the ThreadExecutor + * + * @param runnable the Runnable + * @param delayMillis how long to wait before running it + * @return a Future of the Runnable, already cancelled if the task will not be run + */ @NotNull Future schedule(final @NotNull Runnable runnable, final long delayMillis) throws RejectedExecutionException; diff --git a/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java b/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java index 37778c3cc09..b0ed80a86a1 100644 --- a/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java @@ -2,7 +2,6 @@ import java.util.concurrent.Callable; import java.util.concurrent.Future; -import java.util.concurrent.FutureTask; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @@ -18,17 +17,17 @@ private NoOpSentryExecutorService() {} @Override public @NotNull Future submit(final @NotNull Runnable runnable) { - return new FutureTask<>(() -> null); + return new CancelledFuture<>(); } @Override public @NotNull Future submit(final @NotNull Callable callable) { - return new FutureTask<>(() -> null); + return new CancelledFuture<>(); } @Override public @NotNull Future schedule(@NotNull Runnable runnable, long delayMillis) { - return new FutureTask<>(() -> null); + return new CancelledFuture<>(); } @Override diff --git a/sentry/src/main/java/io/sentry/SentryExecutorService.java b/sentry/src/main/java/io/sentry/SentryExecutorService.java index a469dca5853..e20c0835637 100644 --- a/sentry/src/main/java/io/sentry/SentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/SentryExecutorService.java @@ -2,7 +2,6 @@ import io.sentry.util.AutoClosableReentrantLock; import java.util.concurrent.Callable; -import java.util.concurrent.CancellationException; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledThreadPoolExecutor; @@ -155,31 +154,4 @@ private static final class SentryExecutorServiceThread extends Thread { super(r, name); } } - - private static final class CancelledFuture implements Future { - @Override - public boolean cancel(final boolean mayInterruptIfRunning) { - return true; - } - - @Override - public boolean isCancelled() { - return true; - } - - @Override - public boolean isDone() { - return true; - } - - @Override - public T get() { - throw new CancellationException(); - } - - @Override - public T get(final long timeout, final @NotNull TimeUnit unit) { - throw new CancellationException(); - } - } } diff --git a/sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt index f9df0c52dda..212c1dcde56 100644 --- a/sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt @@ -1,7 +1,10 @@ package io.sentry +import com.google.common.truth.Truth.assertThat import java.util.concurrent.Callable +import java.util.concurrent.CancellationException import kotlin.test.Test +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import org.mockito.kotlin.mock @@ -27,6 +30,18 @@ class NoOpSentryExecutorServiceTest { assertNotNull(future) } + @Test + fun `submitted tasks are reported as cancelled instead of pending forever`() { + assertThat(sut.submit(mock()).isCancelled).isTrue() + assertThat(sut.submit(mock>()).isCancelled).isTrue() + assertThat(sut.schedule(mock(), 0).isCancelled).isTrue() + } + + @Test + fun `getting the result of a dropped task fails fast`() { + assertFailsWith { sut.submit(mock()).get() } + } + @Test fun `close does not throw`() = sut.close(0) @Test