diff --git a/CHANGELOG.md b/CHANGELOG.md index 321ed3ee322..6e13a3a6b5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,10 @@ # Changelog -## Unreleased +### Unreleased -### Fixes +## Fixes +- Fix crash on double SDK init ([#2679](https://github.com/getsentry/sentry-java/pull/2679)) - Track a ttfd span per Activity ([#2673](https://github.com/getsentry/sentry-java/pull/2673)) ## 6.18.0 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java index 5c99612f8de..f2717b4324b 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java @@ -37,6 +37,7 @@ import java.util.Map; import java.util.WeakHashMap; import java.util.concurrent.Future; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -250,11 +251,20 @@ private void startTracing(final @NotNull Activity activity) { final @NotNull ISpan ttfdSpan = transaction.startChild( TTFD_OP, getTtfdDesc(activityName), ttidStartTime, Instrumenter.SENTRY); - ttfdSpanMap.put(activity, ttfdSpan); - ttfdAutoCloseFuture = - options - .getExecutorService() - .schedule(() -> finishExceededTtfdSpan(ttfdSpan, ttidSpan), TTFD_TIMEOUT_MILLIS); + try { + ttfdSpanMap.put(activity, ttfdSpan); + ttfdAutoCloseFuture = + options + .getExecutorService() + .schedule(() -> finishExceededTtfdSpan(ttfdSpan, ttidSpan), TTFD_TIMEOUT_MILLIS); + } catch (RejectedExecutionException e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Time to full display span will not be finished automatically. Did you call Sentry.close()?", + e); + } } // lets bind to the scope so other integrations can pick it up 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 index 912ea9829c8..9322bf50e11 100644 --- 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 @@ -34,9 +34,11 @@ import java.util.UUID; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.TestOnly; final class AndroidTransactionProfiler implements ITransactionProfiler { @@ -77,6 +79,8 @@ final class AndroidTransactionProfiler implements ITransactionProfiler { new ArrayDeque<>(); private final @NotNull Map measurementsMap = new HashMap<>(); + private @Nullable ITransaction currentTransaction = null; + public AndroidTransactionProfiler( final @NotNull Context context, final @NotNull SentryAndroidOptions sentryAndroidOptions, @@ -140,7 +144,16 @@ private void init() { @Override public synchronized void onTransactionStart(final @NotNull ITransaction transaction) { - options.getExecutorService().submit(() -> onTransactionStartSafe(transaction)); + try { + options.getExecutorService().submit(() -> onTransactionStartSafe(transaction)); + } catch (RejectedExecutionException e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Profiling will not be started. Did you call Sentry.close()?", + e); + } } @SuppressLint("NewApi") @@ -235,13 +248,24 @@ public void onFrameMetricCollected( } }); + currentTransaction = transaction; + // We stop profiling after a timeout to avoid huge profiles to be sent - scheduledFinish = - options - .getExecutorService() - .schedule( - () -> timedOutProfilingData = onTransactionFinish(transaction, true, null), - PROFILING_TIMEOUT_MILLIS); + try { + scheduledFinish = + options + .getExecutorService() + .schedule( + () -> timedOutProfilingData = onTransactionFinish(transaction, true, null), + PROFILING_TIMEOUT_MILLIS); + } catch (RejectedExecutionException e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Profiling will not be automatically finished. Did you call Sentry.close()?", + e); + } transactionStartNanos = SystemClock.elapsedRealtimeNanos(); profileStartCpuMillis = Process.getElapsedCpuTime(); @@ -261,10 +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; } @@ -349,6 +382,7 @@ public void onFrameMetricCollected( currentProfilingTransactionData = null; // We clear the counter in case of a timeout transactionsCounter = 0; + currentTransaction = null; if (scheduledFinish != null) { scheduledFinish.cancel(true); @@ -483,6 +517,19 @@ private void putPerformanceCollectionDataInMeasurements( } } + @Override + public void close() { + // we cancel any scheduled work + if (scheduledFinish != null) { + scheduledFinish.cancel(true); + scheduledFinish = null; + } + // we stop profiling + if (currentTransaction != null) { + onTransactionFinish(currentTransaction, true, null); + } + } + /** * Get MemoryInfo object representing the memory state of the application. * @@ -503,4 +550,10 @@ private void putPerformanceCollectionDataInMeasurements( return null; } } + + @TestOnly + @Nullable + ITransaction getCurrentTransaction() { + return currentTransaction; + } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java index 4f150907967..bda83358223 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java @@ -7,6 +7,7 @@ import io.sentry.SentryOptions; import io.sentry.util.Objects; import java.util.concurrent.Future; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import org.jetbrains.annotations.NotNull; @@ -74,6 +75,13 @@ public void register(@NotNull IHub hub, @NotNull SentryOptions options) { } } androidOptions.getLogger().log(SentryLevel.DEBUG, "SendCachedEnvelopeIntegration installed."); + } catch (RejectedExecutionException e) { + androidOptions + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Cached events will not be sent. Did you call Sentry.close()?", + e); } catch (Throwable e) { androidOptions .getLogger() 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 c9b682bbfb3..ce66f1dcc7c 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 @@ -1100,6 +1100,7 @@ class ActivityLifecycleIntegrationTest { return FutureTask {} } override fun close(timeoutMillis: Long) {} + override fun isClosed() = false } fixture.options.tracesSampleRate = 1.0 fixture.options.isEnableTimeToFullDisplayTracing = true @@ -1326,6 +1327,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 index ab5bc2fb296..b72854e7cd7 100644 --- 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 @@ -17,6 +17,7 @@ import io.sentry.TransactionContext import io.sentry.android.core.internal.util.SentryFrameMetricsCollector import io.sentry.profilemeasurements.ProfileMeasurement import io.sentry.test.getCtor +import io.sentry.test.getProperty import org.junit.runner.RunWith import org.mockito.kotlin.any import org.mockito.kotlin.mock @@ -75,6 +76,7 @@ class AndroidTransactionProfilerTest { return FutureTask {} } override fun close(timeoutMillis: Long) {} + override fun isClosed() = false } val options = spy(SentryAndroidOptions()).apply { @@ -150,8 +152,11 @@ class AndroidTransactionProfilerTest { @Test fun `profiler profiles current transaction`() { val profiler = fixture.getSut(context) + assertNull(profiler.currentTransaction) profiler.onTransactionStart(fixture.transaction1) + assertNotNull(profiler.currentTransaction) val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null) + assertNull(profiler.currentTransaction) assertNotNull(profilingTraceData) assertEquals(profilingTraceData.transactionId, fixture.transaction1.eventId.toString()) @@ -401,4 +406,22 @@ class AndroidTransactionProfilerTest { data.measurementsMap[ProfileMeasurement.ID_MEMORY_NATIVE_FOOTPRINT]!!.values.map { it.value } ) } + + @Test + fun `profiler stops profiling, clear current transaction and scheduled job on close`() { + val profiler = fixture.getSut(context) + profiler.onTransactionStart(fixture.transaction1) + assertNotNull(profiler.currentTransaction) + + profiler.close() + assertNull(profiler.currentTransaction) + + // The timeout scheduled job should be cleared + val scheduledJob = profiler.getProperty?>("scheduledFinish") + assertNull(scheduledJob) + + // Calling transaction finish returns null, as the profiler was stopped + val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null) + assertNull(profilingTraceData) + } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt index 5cee3e792de..f6a4a995060 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt @@ -115,5 +115,7 @@ class SentryAndroidOptionsTest { transaction: ITransaction, performanceCollectionData: List? ): ProfilingTraceData? = null + + override fun close() {} } } diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt new file mode 100644 index 00000000000..39bc21c4e9d --- /dev/null +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt @@ -0,0 +1,77 @@ +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() { + + @Test + fun doubleInitDoesNotThrow() { + initSentry(false) { options: SentryAndroidOptions -> + options.tracesSampleRate = 1.0 + options.profilesSampleRate = 1.0 + } + val transaction = Sentry.startTransaction("e2etests", "testInit") + val sampleScenario = launchActivity() + initSentry(false) { options: SentryAndroidOptions -> + options.tracesSampleRate = 1.0 + options.profilesSampleRate = 1.0 + } + transaction.finish() + sampleScenario.moveToState(Lifecycle.State.DESTROYED) + 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() + 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(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 index b44704316b2..666fc6080b6 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -188,6 +188,7 @@ public final class io/sentry/DateUtils { public final class io/sentry/DefaultTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector { public fun (Lio/sentry/SentryOptions;)V + public fun close ()V public fun start (Lio/sentry/ITransaction;)V public fun stop (Lio/sentry/ITransaction;)Ljava/util/List; } @@ -531,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; @@ -597,6 +599,7 @@ public abstract interface class io/sentry/ITransaction : io/sentry/ISpan { } public abstract interface class io/sentry/ITransactionProfiler { + public abstract fun close ()V public abstract fun onTransactionFinish (Lio/sentry/ITransaction;Ljava/util/List;)Lio/sentry/ProfilingTraceData; public abstract fun onTransactionStart (Lio/sentry/ITransaction;)V } @@ -905,12 +908,14 @@ public final class io/sentry/NoOpTransaction : io/sentry/ITransaction { } public final class io/sentry/NoOpTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector { + public fun close ()V public static fun getInstance ()Lio/sentry/NoOpTransactionPerformanceCollector; public fun start (Lio/sentry/ITransaction;)V public fun stop (Lio/sentry/ITransaction;)Ljava/util/List; } public final class io/sentry/NoOpTransactionProfiler : io/sentry/ITransactionProfiler { + public fun close ()V public static fun getInstance ()Lio/sentry/NoOpTransactionProfiler; public fun onTransactionFinish (Lio/sentry/ITransaction;Ljava/util/List;)Lio/sentry/ProfilingTraceData; public fun onTransactionStart (Lio/sentry/ITransaction;)V @@ -2151,6 +2156,7 @@ public final class io/sentry/TransactionOptions : io/sentry/SpanOptions { } public abstract interface class io/sentry/TransactionPerformanceCollector { + public abstract fun close ()V public abstract fun start (Lio/sentry/ITransaction;)V public abstract fun stop (Lio/sentry/ITransaction;)Ljava/util/List; } diff --git a/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java b/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java index b4ffec08721..eb11f2a779d 100644 --- a/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java +++ b/sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java @@ -7,6 +7,7 @@ import java.util.Timer; import java.util.TimerTask; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @@ -45,9 +46,18 @@ public void start(final @NotNull ITransaction transaction) { if (!performanceDataMap.containsKey(transaction.getEventId().toString())) { performanceDataMap.put(transaction.getEventId().toString(), new ArrayList<>()); // We schedule deletion of collected performance data after a timeout - options - .getExecutorService() - .schedule(() -> stop(transaction), TRANSACTION_COLLECTION_TIMEOUT_MILLIS); + try { + options + .getExecutorService() + .schedule(() -> stop(transaction), TRANSACTION_COLLECTION_TIMEOUT_MILLIS); + } catch (RejectedExecutionException e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Performance collector will not be automatically finished. Did you call Sentry.close()?", + e); + } } if (!isStarted.getAndSet(true)) { synchronized (timerLock) { @@ -113,4 +123,20 @@ public void run() { } return data; } + + @Override + public void close() { + performanceDataMap.clear(); + options + .getLogger() + .log(SentryLevel.DEBUG, "stop collecting all performance info for transactions"); + if (isStarted.getAndSet(false)) { + synchronized (timerLock) { + if (timer != null) { + timer.cancel(); + timer = null; + } + } + } + } } diff --git a/sentry/src/main/java/io/sentry/Hub.java b/sentry/src/main/java/io/sentry/Hub.java index e22259195da..dcc80fa2877 100644 --- a/sentry/src/main/java/io/sentry/Hub.java +++ b/sentry/src/main/java/io/sentry/Hub.java @@ -339,6 +339,10 @@ public void close() { ((Closeable) integration).close(); } } + + withScope(scope -> scope.clear()); + options.getTransactionProfiler().close(); + options.getTransactionPerformanceCollector().close(); options.getExecutorService().close(options.getShutdownTimeoutMillis()); // Close the top-most client diff --git a/sentry/src/main/java/io/sentry/ISentryExecutorService.java b/sentry/src/main/java/io/sentry/ISentryExecutorService.java index 7f6c81cedce..9bdef8db2b7 100644 --- a/sentry/src/main/java/io/sentry/ISentryExecutorService.java +++ b/sentry/src/main/java/io/sentry/ISentryExecutorService.java @@ -2,6 +2,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.Future; +import java.util.concurrent.RejectedExecutionException; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @@ -16,7 +17,7 @@ public interface ISentryExecutorService { * @return a Future of the Runnable */ @NotNull - Future submit(final @NotNull Runnable runnable); + Future submit(final @NotNull Runnable runnable) throws RejectedExecutionException; /** * Submits a Callable to the ThreadExecutor @@ -25,10 +26,11 @@ public interface ISentryExecutorService { * @return a Future of the Callable */ @NotNull - Future submit(final @NotNull Callable callable); + Future submit(final @NotNull Callable callable) throws RejectedExecutionException; @NotNull - Future schedule(final @NotNull Runnable runnable, final long delayMillis); + Future schedule(final @NotNull Runnable runnable, final long delayMillis) + throws RejectedExecutionException; /** * Closes the ThreadExecutor and awaits for the timeout @@ -36,4 +38,11 @@ public interface ISentryExecutorService { * @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/ITransactionProfiler.java b/sentry/src/main/java/io/sentry/ITransactionProfiler.java index 44676e6f727..a5eb1c18843 100644 --- a/sentry/src/main/java/io/sentry/ITransactionProfiler.java +++ b/sentry/src/main/java/io/sentry/ITransactionProfiler.java @@ -14,4 +14,7 @@ public interface ITransactionProfiler { ProfilingTraceData onTransactionFinish( @NotNull ITransaction transaction, @Nullable List performanceCollectionData); + + /** Cancel the profiler and stops it. Used on SDK close. */ + void close(); } diff --git a/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java b/sentry/src/main/java/io/sentry/NoOpSentryExecutorService.java index 232dfaf112e..735c0dc29a3 100644 --- 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/NoOpTransactionPerformanceCollector.java b/sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java index 02edeb10944..ea8ef769852 100644 --- a/sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java +++ b/sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java @@ -22,4 +22,7 @@ public void start(@NotNull ITransaction transaction) {} public @Nullable List stop(@NotNull ITransaction transaction) { return null; } + + @Override + public void close() {} } diff --git a/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java b/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java index 9f51ea33a97..f4c5ac07d8a 100644 --- a/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java +++ b/sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java @@ -23,4 +23,7 @@ public void onTransactionStart(@NotNull ITransaction transaction) {} @Nullable List performanceCollectionData) { return null; } + + @Override + public void close() {} } diff --git a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java index 2646d9be398..01f9b960183 100644 --- a/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java +++ b/sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java @@ -2,6 +2,7 @@ import io.sentry.util.Objects; import java.io.File; +import java.util.concurrent.RejectedExecutionException; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -88,6 +89,13 @@ public final void register(final @NotNull IHub hub, final @NotNull SentryOptions .getLogger() .log(SentryLevel.DEBUG, "SendCachedEventFireAndForgetIntegration installed."); addIntegrationToSdkVersion(); + } catch (RejectedExecutionException e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Cached events will not be sent. Did you call Sentry.close()?", + e); } catch (Throwable e) { options .getLogger() diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index 4b420a19a70..c3eea1d9b1c 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -19,6 +19,7 @@ import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.List; +import java.util.concurrent.RejectedExecutionException; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -216,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. @@ -280,17 +289,26 @@ private static boolean initConfigurations(final @NotNull SentryOptions options) profilingTracesDir.mkdirs(); final File[] oldTracesDirContent = profilingTracesDir.listFiles(); - options - .getExecutorService() - .submit( - () -> { - if (oldTracesDirContent == null) return; - // Method trace files are normally deleted at the end of traces, but if that fails - // for some reason we try to clear any old files here. - for (File f : oldTracesDirContent) { - FileUtils.deleteRecursively(f); - } - }); + try { + options + .getExecutorService() + .submit( + () -> { + if (oldTracesDirContent == null) return; + // Method trace files are normally deleted at the end of traces, but if that fails + // for some reason we try to clear any old files here. + for (File f : oldTracesDirContent) { + FileUtils.deleteRecursively(f); + } + }); + } catch (RejectedExecutionException e) { + options + .getLogger() + .log( + SentryLevel.ERROR, + "Failed to call the executor. Old profiles will not be deleted. Did you call Sentry.close()?", + e); + } } final @NotNull IModulesLoader modulesLoader = options.getModulesLoader(); diff --git a/sentry/src/main/java/io/sentry/SentryExecutorService.java b/sentry/src/main/java/io/sentry/SentryExecutorService.java index 89e343d4758..ba0ae4fac56 100644 --- 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/main/java/io/sentry/TransactionPerformanceCollector.java b/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java index 013667d2cfd..eec2c35fee3 100644 --- a/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java +++ b/sentry/src/main/java/io/sentry/TransactionPerformanceCollector.java @@ -1,6 +1,7 @@ package io.sentry; import java.util.List; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -10,4 +11,8 @@ public interface TransactionPerformanceCollector { @Nullable List stop(@NotNull ITransaction transaction); + + /** Cancel the collector and stops it. Used on SDK close. */ + @ApiStatus.Internal + void close(); } diff --git a/sentry/src/test/java/io/sentry/DefaultTransactionPerformanceCollectorTest.kt b/sentry/src/test/java/io/sentry/DefaultTransactionPerformanceCollectorTest.kt index b87de67f68f..edcbf32811f 100644 --- 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 { @@ -226,6 +227,20 @@ class DefaultTransactionPerformanceCollectorTest { verify(threadCheckerCollector, atLeast(1)).collect(any()) } + @Test + fun `when close, timer is stopped and data is cleared`() { + val collector = fixture.getSut() + collector.start(fixture.transaction1) + collector.close() + + // Timer was canceled + verify(fixture.mockTimer)!!.scheduleAtFixedRate(any(), any(), eq(100)) + verify(fixture.mockTimer)!!.cancel() + + // Data was cleared + assertNull(collector.stop(fixture.transaction1)) + } + inner class ThreadCheckerCollector : ICollector { override fun setup() { if (mainThreadChecker.isMainThread) { diff --git a/sentry/src/test/java/io/sentry/HubTest.kt b/sentry/src/test/java/io/sentry/HubTest.kt index 9e6d564caeb..62ad641372f 100644 --- a/sentry/src/test/java/io/sentry/HubTest.kt +++ b/sentry/src/test/java/io/sentry/HubTest.kt @@ -1530,16 +1530,22 @@ class HubTest { } @Test - fun `Hub should close the sentry executor processor on close call`() { + fun `Hub should close the sentry executor processor, profiler and performance collector on close call`() { val executor = mock() + val profiler = mock() + val performanceCollector = mock() val options = SentryOptions().apply { dsn = "https://key@sentry.io/proj" cacheDirPath = file.absolutePath executorService = executor + setTransactionProfiler(profiler) + transactionPerformanceCollector = performanceCollector } val sut = Hub(options) sut.close() verify(executor).close(any()) + verify(profiler).close() + verify(performanceCollector).close() } @Test diff --git a/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt b/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt index 288380f05ed..e210226c341 100644 --- a/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt +++ b/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt @@ -14,4 +14,8 @@ class NoOpTransactionProfilerTest { fun `onTransactionFinish does returns null`() { assertNull(profiler.onTransactionFinish(NoOpTransaction.getInstance(), null)) } + + @Test + fun `close does not throw`() = + profiler.close() } diff --git a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt index f06379008bf..1cd00109004 100644 --- 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() + 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() + 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 index a84cf32dcc0..5c95eb46eda 100644 --- 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() + 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() + 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 } diff --git a/sentry/src/test/java/io/sentry/SentryTracerTest.kt b/sentry/src/test/java/io/sentry/SentryTracerTest.kt index 87ffb165d8f..fafe1f40090 100644 --- a/sentry/src/test/java/io/sentry/SentryTracerTest.kt +++ b/sentry/src/test/java/io/sentry/SentryTracerTest.kt @@ -1069,6 +1069,7 @@ class SentryTracerTest { val mockPerformanceCollector = object : TransactionPerformanceCollector { override fun start(transaction: ITransaction) {} override fun stop(transaction: ITransaction): MutableList = data + override fun close() {} } val transaction = fixture.getSut(optionsConfiguration = { it.profilesSampleRate = 1.0