added ISentryExecutorService.close internal method

profiling is now stopped on executorService exceptions transaction bound to the scope is not cancelled automatically anymore on Sentry.close() ISentryExecutorService is re-init in Sentry.init if it was shutdown
getsentry · Apr 28, 2023 · 7e1be4f · 7e1be4f
1 parent 2914bec
commit 7e1be4f
Show file tree

Hide file tree

Showing 14 changed files with 133 additions and 35 deletions.
diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java
@@ -285,17 +285,19 @@ public void onFrameMetricCollected(
           .getExecutorService()
           .submit(() -> onTransactionFinish(transaction, false, performanceCollectionData))
           .get();
-    } catch (ExecutionException e) {
-      options.getLogger().log(SentryLevel.ERROR, "Error finishing profiling: ", e);
-    } catch (InterruptedException e) {
+    } catch (ExecutionException | InterruptedException e) {
       options.getLogger().log(SentryLevel.ERROR, "Error finishing profiling: ", e);
+      // We stop profiling even on exceptions, so profiles don't run indefinitely
+      onTransactionFinish(transaction, false, null);
     } catch (RejectedExecutionException e) {
       options
           .getLogger()
           .log(
               SentryLevel.ERROR,
               "Failed to call the executor. Profiling could not be finished. Did you call Sentry.close()?",
               e);
+      // We stop profiling even on exceptions, so profiles don't run indefinitely
+      onTransactionFinish(transaction, false, null);
     }
     return null;
   }

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
@@ -1097,6 +1097,7 @@ class ActivityLifecycleIntegrationTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
         fixture.options.tracesSampleRate = 1.0
         fixture.options.isEnableTimeToFullDisplayTracing = true
@@ -1321,6 +1322,7 @@ class ActivityLifecycleIntegrationTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
         fixture.options.tracesSampleRate = 1.0
         fixture.options.isEnableTimeToFullDisplayTracing = true

diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt
@@ -76,6 +76,7 @@ class AndroidTransactionProfilerTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
 
         val options = spy(SentryAndroidOptions()).apply {

diff --git a/...tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt b/...tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt
@@ -3,10 +3,14 @@ package io.sentry.uitest.android
 import androidx.lifecycle.Lifecycle
 import androidx.test.core.app.launchActivity
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import io.sentry.ProfilingTraceData
 import io.sentry.Sentry
 import io.sentry.android.core.SentryAndroidOptions
+import io.sentry.assertEnvelopeItem
+import io.sentry.protocol.SentryTransaction
 import org.junit.runner.RunWith
 import kotlin.test.Test
+import kotlin.test.assertEquals
 
 @RunWith(AndroidJUnit4::class)
 class SdkInitTests : BaseUiTest() {
@@ -28,4 +32,46 @@ class SdkInitTests : BaseUiTest() {
         val transaction2 = Sentry.startTransaction("e2etests", "testInit")
         transaction2.finish()
     }
+
+    @Test
+    fun doubleInitWithSameOptionsDoesNotThrow() {
+        val options = SentryAndroidOptions()
+
+        initSentry(true) {
+            it.tracesSampleRate = 1.0
+            it.profilesSampleRate = 1.0
+            // We use the same executorService before and after closing the SDK
+            it.executorService = options.executorService
+            it.isDebug = true
+        }
+        val transaction = Sentry.startTransaction("e2etests", "testInit")
+        val sampleScenario = launchActivity<EmptyActivity>()
+        initSentry(true) {
+            it.tracesSampleRate = 1.0
+            it.profilesSampleRate = 1.0
+            // We use the same executorService before and after closing the SDK
+            it.executorService = options.executorService
+            it.isDebug = true
+        }
+        relayIdlingResource.increment()
+        transaction.finish()
+        sampleScenario.moveToState(Lifecycle.State.DESTROYED)
+        val transaction2 = Sentry.startTransaction("e2etests2", "testInit")
+        transaction2.finish()
+
+        relay.assert {
+            findEnvelope {
+                assertEnvelopeItem<SentryTransaction>(it.items.toList()).transaction == "e2etests2"
+            }.assert {
+                val transactionItem: SentryTransaction = it.assertItem()
+                // Profiling uses executorService, so if the executorService is shutdown it would fail
+                val profilingTraceData: ProfilingTraceData = it.assertItem()
+                it.assertNoOtherItems()
+                assertEquals("e2etests2", transactionItem.transaction)
+                assertEquals("e2etests2", profilingTraceData.transactionName)
+            }
+            assertNoOtherEnvelopes()
+            assertNoOtherRequests()
+        }
+    }
 }
diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -532,6 +532,7 @@ public abstract interface class io/sentry/ISentryClient {
 
 public abstract interface class io/sentry/ISentryExecutorService {
 	public abstract fun close (J)V
+	public abstract fun isClosed ()Z
 	public abstract fun schedule (Ljava/lang/Runnable;J)Ljava/util/concurrent/Future;
 	public abstract fun submit (Ljava/lang/Runnable;)Ljava/util/concurrent/Future;
 	public abstract fun submit (Ljava/util/concurrent/Callable;)Ljava/util/concurrent/Future;

diff --git a/sentry/src/main/java/io/sentry/Hub.java b/sentry/src/main/java/io/sentry/Hub.java
@@ -340,18 +340,7 @@ public void close() {
           }
         }
 
-        withScope(
-            scope -> {
-              ITransaction transaction = scope.getTransaction();
-              if (transaction != null) {
-                for (Span span : transaction.getSpans()) {
-                  span.setSpanFinishedCallback(null);
-                  span.finish(SpanStatus.CANCELLED);
-                }
-                transaction.finish(SpanStatus.CANCELLED);
-              }
-              scope.clear();
-            });
+        withScope(scope -> scope.clear());
         options.getTransactionProfiler().close();
         options.getTransactionPerformanceCollector().close();
         options.getExecutorService().close(options.getShutdownTimeoutMillis());

diff --git a/sentry/src/main/java/io/sentry/ISentryExecutorService.java b/sentry/src/main/java/io/sentry/ISentryExecutorService.java
@@ -38,4 +38,11 @@ Future<?> schedule(final @NotNull Runnable runnable, final long delayMillis)
    * @param timeoutMillis the timeout in millis
    */
   void close(long timeoutMillis);
+
+  /**
+   * Check if there was a previous call to the close() method.
+   *
+   * @return If the executorService was previously closed
+   */
+  boolean isClosed();
 }
diff --git a/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java b/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java
@@ -31,4 +31,9 @@ private NoOpSentryExecutorService() {}
 
   @Override
   public void close(long timeoutMillis) {}
+
+  @Override
+  public boolean isClosed() {
+    return false;
+  }
 }
diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java
@@ -217,6 +217,14 @@ private static synchronized void init(
 
     hub.close();
 
+    // If the executorService passed in the init is the same that was previously closed, we have to
+    // set a new one
+    final ISentryExecutorService sentryExecutorService = options.getExecutorService();
+    // If the passed executor service was previously called we set a new one
+    if (sentryExecutorService.isClosed()) {
+      options.setExecutorService(new SentryExecutorService());
+    }
+
     // when integrations are registered on Hub ctor and async integrations are fired,
     // it might and actually happened that integrations called captureSomething
     // and hub was still NoOp.

diff --git a/sentry/src/main/java/io/sentry/SentryExecutorService.java b/sentry/src/main/java/io/sentry/SentryExecutorService.java
@@ -52,4 +52,11 @@ public void close(final long timeoutMillis) {
       }
     }
   }
+
+  @Override
+  public boolean isClosed() {
+    synchronized (executorService) {
+      return executorService.isShutdown();
+    }
+  }
 }
diff --git a/sentry/src/test/java/io/sentry/DefaultTransactionPerformanceCollectorTest.kt b/sentry/src/test/java/io/sentry/DefaultTransactionPerformanceCollectorTest.kt
@@ -47,6 +47,7 @@ class DefaultTransactionPerformanceCollectorTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
 
         val mockCpuCollector: ICollector = object : ICollector {

diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt
@@ -1548,26 +1548,6 @@ class HubTest {
         verify(performanceCollector).close()
     }
 
-    @Test
-    fun `Hub should cancel current transaction bound to the scope and its spans`() {
-        val hub = generateHub {
-            it.tracesSampleRate = 1.0
-        }
-        val transaction = hub.startTransaction("test", "test", true)
-        val span = transaction.startChild("span1")
-        val span2 = transaction.startChild("span1")
-        assertFalse(transaction.isFinished)
-        assertFalse(span.isFinished)
-        assertFalse(span2.isFinished)
-        hub.close()
-        assertTrue(transaction.isFinished)
-        assertTrue(span.isFinished)
-        assertTrue(span2.isFinished)
-        assertEquals(SpanStatus.CANCELLED, transaction.status)
-        assertEquals(SpanStatus.CANCELLED, span.status)
-        assertEquals(SpanStatus.CANCELLED, span2.status)
-    }
-
     @Test
     fun `when tracesSampleRate and tracesSampler are not set on SentryOptions, startTransaction returns NoOp`() {
         val hub = generateHub {

diff --git a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt
@@ -10,6 +10,7 @@ import java.util.concurrent.Executors
 import java.util.concurrent.ScheduledExecutorService
 import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.test.Test
+import kotlin.test.assertFalse
 import kotlin.test.assertTrue
 
 class SentryExecutorServiceTest {
@@ -87,4 +88,20 @@ class SentryExecutorServiceTest {
         await.untilFalse(atomicBoolean)
         sentryExecutor.close(15000)
     }
+
+    @Test
+    fun `SentryExecutorService isClosed returns true if executor is shutdown`() {
+        val executor = mock<ScheduledExecutorService>()
+        val sentryExecutor = SentryExecutorService(executor)
+        whenever(executor.isShutdown).thenReturn(true)
+        assertTrue(sentryExecutor.isClosed)
+    }
+
+    @Test
+    fun `SentryExecutorService isClosed returns false if executor is not shutdown`() {
+        val executor = mock<ScheduledExecutorService>()
+        val sentryExecutor = SentryExecutorService(executor)
+        whenever(executor.isShutdown).thenReturn(false)
+        assertFalse(sentryExecutor.isClosed)
+    }
 }
diff --git a/sentry/src/test/java/io/sentry/SentryTest.kt b/sentry/src/test/java/io/sentry/SentryTest.kt
@@ -13,6 +13,7 @@ import org.mockito.kotlin.argThat
 import org.mockito.kotlin.eq
 import org.mockito.kotlin.mock
 import org.mockito.kotlin.verify
+import org.mockito.kotlin.whenever
 import java.io.File
 import java.nio.file.Files
 import java.util.concurrent.CompletableFuture
@@ -21,6 +22,7 @@ import kotlin.test.BeforeTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
+import kotlin.test.assertNotEquals
 import kotlin.test.assertNotNull
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
@@ -500,6 +502,36 @@ class SentryTest {
         verify(hub).reportFullyDisplayed()
     }
 
+    @Test
+    fun `ignores executorService if it is closed`() {
+        var sentryOptions: SentryOptions? = null
+        val executorService = mock<ISentryExecutorService>()
+        whenever(executorService.isClosed).thenReturn(true)
+
+        Sentry.init {
+            it.dsn = dsn
+            it.executorService = executorService
+            sentryOptions = it
+        }
+
+        assertNotEquals(executorService, sentryOptions!!.executorService)
+    }
+
+    @Test
+    fun `accept executorService if it is not closed`() {
+        var sentryOptions: SentryOptions? = null
+        val executorService = mock<ISentryExecutorService>()
+        whenever(executorService.isClosed).thenReturn(false)
+
+        Sentry.init {
+            it.dsn = dsn
+            it.executorService = executorService
+            sentryOptions = it
+        }
+
+        assertEquals(executorService, sentryOptions!!.executorService)
+    }
+
     private class CustomMainThreadChecker : IMainThreadChecker {
         override fun isMainThread(threadId: Long): Boolean = false
     }