Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash on double SDK init #2679

Merged
merged 6 commits into from
Apr 28, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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
stefanosiano committed Apr 28, 2023
commit 7e1be4f3a6808c3bdaefe90e73aad0cf6410f728
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -76,6 +76,7 @@ class AndroidTransactionProfilerTest {
return FutureTask {}
}
override fun close(timeoutMillis: Long) {}
override fun isClosed() = false
}

val options = spy(SentryAndroidOptions()).apply {
Original file line number Diff line number Diff line change
@@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I think there's one more test to cover which would fail right now: Initializing the SDK with the same options twice.

val options = {}
Sentry.init(options)
Sentry.close()
Sentry.init(options)

I guess the solution here would be to re-init the executor-service in .init()

}

@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()
}
}
}
1 change: 1 addition & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
@@ -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;
13 changes: 1 addition & 12 deletions sentry/src/main/java/io/sentry/Hub.java
Original file line number Diff line number Diff line change
@@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

late to the party, but this is not gonna do much, because withScope will duplicate the scope, and then we're trying to clear the duplicated scope, but the main scope stays untouched. This should be changed to configureScope. I think we need a custom lint rule to never use withScope on mobile 😅 this is the second time we introduced something like this (this one is not as severe though).

options.getTransactionProfiler().close();
options.getTransactionPerformanceCollector().close();
options.getExecutorService().close(options.getShutdownTimeoutMillis());
7 changes: 7 additions & 0 deletions sentry/src/main/java/io/sentry/ISentryExecutorService.java
Original file line number Diff line number Diff line change
@@ -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();
}
Original file line number Diff line number Diff line change
@@ -31,4 +31,9 @@ private NoOpSentryExecutorService() {}

@Override
public void close(long timeoutMillis) {}

@Override
public boolean isClosed() {
return false;
}
}
8 changes: 8 additions & 0 deletions sentry/src/main/java/io/sentry/Sentry.java
Original file line number Diff line number Diff line change
@@ -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.
7 changes: 7 additions & 0 deletions sentry/src/main/java/io/sentry/SentryExecutorService.java
Original file line number Diff line number Diff line change
@@ -52,4 +52,11 @@ public void close(final long timeoutMillis) {
}
}
}

@Override
public boolean isClosed() {
synchronized (executorService) {
return executorService.isShutdown();
}
}
}
Original file line number Diff line number Diff line change
@@ -47,6 +47,7 @@ class DefaultTransactionPerformanceCollectorTest {
return FutureTask {}
}
override fun close(timeoutMillis: Long) {}
override fun isClosed() = false
}

val mockCpuCollector: ICollector = object : ICollector {
20 changes: 0 additions & 20 deletions sentry/src/test/java/io/sentry/HubTest.kt
Original file line number Diff line number Diff line change
@@ -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 {
17 changes: 17 additions & 0 deletions sentry/src/test/java/io/sentry/SentryExecutorServiceTest.kt
Original file line number Diff line number Diff line change
@@ -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)
}
}
32 changes: 32 additions & 0 deletions sentry/src/test/java/io/sentry/SentryTest.kt
Original file line number Diff line number Diff line change
@@ -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
}