fix(core): Report no-op executor tasks as cancelled

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
 

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<Activity>()
     val activity2 = mock<Activity>()

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<Void> { 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 <T> cancelledFuture(): FutureTask<T> = FutureTask<T> { null }.apply { cancel(false) }
 
-  override fun <T> submit(callable: Callable<T>): Future<T> = FutureTask<T> { null }
+  override fun submit(runnable: Runnable): Future<*> = cancelledFuture<Void>()
 
-  override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> =
-    FutureTask<Void> { null }
+  override fun <T> submit(callable: Callable<T>): Future<T> = cancelledFuture()
+
+  override fun schedule(runnable: Runnable, delayMillis: Long): Future<*> = cancelledFuture<Void>()
 
   override fun close(timeoutMillis: Long) {}
 

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<T> implements Future<T> {
+  @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();
+  }
+}

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

@@ -6,15 +6,23 @@
 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.
+ *
+ * <p>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 {
 
   /**
    * 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
   <T> Future<T> submit(final @NotNull Callable<T> 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;

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 <T> Future<T> submit(final @NotNull Callable<T> 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

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<T> implements Future<T> {
-    @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();
-    }
-  }
 }

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<Runnable>()).isCancelled).isTrue()
+    assertThat(sut.submit(mock<Callable<*>>()).isCancelled).isTrue()
+    assertThat(sut.schedule(mock(), 0).isCancelled).isTrue()
+  }
+
+  @Test
+  fun `getting the result of a dropped task fails fast`() {
+    assertFailsWith<CancellationException> { sut.submit(mock<Runnable>()).get() }
+  }
+
   @Test fun `close does not throw`() = sut.close(0)
 
   @Test