diff --git a/CHANGELOG.md b/CHANGELOG.md index 321ed3ee32..6e13a3a6b5 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 5c99612f8d..f2717b4324 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 912ea9829c..9322bf50e1 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 4f15090796..bda8335822 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 c9b682bbfb..ce66f1dcc7 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 ab5bc2fb29..b72854e7cd 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 5cee3e792d..f6a4a99506 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 0000000000..39bc21c4e9 --- /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 b44704316b..666fc6080b 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 b4ffec0872..eb11f2a779 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 e22259195d..dcc80fa287 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 7f6c81cedc..9bdef8db2b 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 44676e6f72..a5eb1c1884 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 232dfaf112..735c0dc29a 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 02edeb1094..ea8ef76985 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 9f51ea33a9..f4c5ac07d8 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 2646d9be39..01f9b96018 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 4b420a19a7..c3eea1d9b1 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 89e343d475..ba0ae4fac5 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 013667d2cf..eec2c35fee 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 b87de67f68..e132839da1 100644 --- a/sentry/src/test/java/io/sentry/DefaultTransactionPerformanceCollectorTest.kt +++ b/sentry/src/test/java/io/sentry/DefaultTransactionPerformanceCollectorTest.kt @@ -16,6 +16,7 @@ import java.util.Timer import java.util.concurrent.Callable import java.util.concurrent.Future import java.util.concurrent.FutureTask +import java.util.concurrent.RejectedExecutionException import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith @@ -47,6 +48,7 @@ class DefaultTransactionPerformanceCollectorTest { return FutureTask {} } override fun close(timeoutMillis: Long) {} + override fun isClosed() = false } val mockCpuCollector: ICollector = object : ICollector { @@ -60,9 +62,9 @@ class DefaultTransactionPerformanceCollectorTest { whenever(hub.options).thenReturn(options) } - fun getSut(memoryCollector: ICollector? = JavaMemoryCollector(), cpuCollector: ICollector? = mockCpuCollector): TransactionPerformanceCollector { + fun getSut(memoryCollector: ICollector? = JavaMemoryCollector(), cpuCollector: ICollector? = mockCpuCollector, executorService: ISentryExecutorService = mockExecutorService): TransactionPerformanceCollector { options.dsn = "https://key@sentry.io/proj" - options.executorService = mockExecutorService + options.executorService = executorService if (cpuCollector != null) { options.addCollector(cpuCollector) } @@ -226,6 +228,32 @@ 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)) + } + + @Test + fun `start does not throw on executor shut down`() { + val executorService = mock() + whenever(executorService.schedule(any(), any())).thenThrow(RejectedExecutionException()) + val logger = mock() + fixture.options.setLogger(logger) + fixture.options.isDebug = true + val sut = fixture.getSut(executorService = executorService) + sut.start(fixture.transaction1) + verify(logger).log(eq(SentryLevel.ERROR), eq("Failed to call the executor. Performance collector will not be automatically finished. Did you call Sentry.close()?"), any()) + } + 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 9e6d564cae..62ad641372 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/NoOpSentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt new file mode 100644 index 0000000000..d0ce9ab7cb --- /dev/null +++ b/sentry/src/test/java/io/sentry/NoOpSentryExecutorServiceTest.kt @@ -0,0 +1,37 @@ +package io.sentry + +import org.mockito.kotlin.mock +import java.util.concurrent.Callable +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertNotNull + +class NoOpSentryExecutorServiceTest { + private var sut: ISentryExecutorService = NoOpSentryExecutorService.getInstance() + + @Test + fun `submit runnable returns a Future`() { + val future = sut.submit(mock()) + assertNotNull(future) + } + + @Test + fun `submit callable returns a Future`() { + val future = sut.submit(mock>()) + assertNotNull(future) + } + + @Test + fun `schedule returns a Future`() { + val future = sut.submit(mock>()) + assertNotNull(future) + } + + @Test + fun `close does not throw`() = sut.close(0) + + @Test + fun `isClosed returns false`() { + assertFalse(sut.isClosed) + } +} diff --git a/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt b/sentry/src/test/java/io/sentry/NoOpTransactionProfilerTest.kt index 288380f05e..e210226c34 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/SendCachedEnvelopeFireAndForgetIntegrationTest.kt b/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt index e1f6982748..a1c06d31db 100644 --- a/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt +++ b/sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt @@ -82,6 +82,16 @@ class SendCachedEnvelopeFireAndForgetIntegrationTest { assert(fixture.options.sdkVersion!!.integrationSet.contains("SendCachedEnvelopeFireAndForget")) } + @Test + fun `register does not throw on executor shut down`() { + fixture.options.cacheDirPath = "cache" + fixture.options.executorService.close(0) + whenever(fixture.callback.create(any(), any())).thenReturn(mock()) + val sut = fixture.getSut() + sut.register(fixture.hub, fixture.options) + verify(fixture.logger).log(eq(SentryLevel.ERROR), eq("Failed to call the executor. Cached events will not be sent. Did you call Sentry.close()?"), any()) + } + private class CustomFactory : SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory { override fun create(hub: IHub, options: SentryOptions): SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget? { return null diff --git a/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt b/sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt index f06379008b..1cd0010900 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 a84cf32dcc..0dfe70bf1f 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 @@ -478,6 +480,21 @@ class SentryTest { assertTrue { sentryOptions!!.collectors.any { it is CustomMemoryCollector } } } + @Test + fun `init does not throw on executor shut down`() { + val logger = mock() + + Sentry.init { + it.dsn = dsn + it.profilesSampleRate = 1.0 + it.cacheDirPath = getTempPath() + it.setLogger(logger) + it.executorService.close(0) + it.isDebug = true + } + verify(logger).log(eq(SentryLevel.ERROR), eq("Failed to call the executor. Old profiles will not be deleted. Did you call Sentry.close()?"), any()) + } + @Test fun `reportFullyDisplayed calls hub reportFullyDisplayed`() { val hub = mock() @@ -500,6 +517,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 87ffb165d8..fafe1f4009 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