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.

Sign up for GitHub

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

Fix crash on double SDK init #2679

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
960fd20
all calls to ISentryExecutorService are now wrapped in a try catch block
stefanosiano Apr 27, 2023
6af6003
updated changelog
stefanosiano Apr 27, 2023
2914bec
updated exception description
stefanosiano Apr 28, 2023
7e1be4f
added ISentryExecutorService.close internal method
stefanosiano Apr 28, 2023
997f4e3
Merge branch 'main' into fix/multiple-sdk-init
stefanosiano Apr 28, 2023
c0607ab
added tests for codecov
stefanosiano Apr 28, 2023
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

### Unreleased

## Fixes

- Fix crash on double SDK init ([#2679](https://github.com/getsentry/sentry-java/pull/2679))

## 6.18.0

### Features
Expand Down
19 changes: 14 additions & 5 deletions sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,20 @@ private void startTracing(final @NotNull Activity activity) {
ttfdSpan =
transaction.startChild(
TTFD_OP, getTtfdDesc(activityName), ttidStartTime, Instrumenter.SENTRY);
ttfdAutoCloseFuture =
options
.getExecutorService()
.schedule(
() -> finishExceededTtfdSpan(ttidSpanMap.get(activity)), TTFD_TIMEOUT_MILLIS);
try {
ttfdAutoCloseFuture =
options
.getExecutorService()
.schedule(
() -> finishExceededTtfdSpan(ttidSpanMap.get(activity)), TTFD_TIMEOUT_MILLIS);
} catch (Throwable e) {
options
.getLogger()
.log(
SentryLevel.ERROR,
"Failed to call the executor. Time to full display span will not be finished automatically.",
e);
}
}

// lets bind to the scope so other integrations can pick it up
Expand Down
59 changes: 52 additions & 7 deletions sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
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 {

Expand Down Expand Up @@ -77,6 +78,8 @@ final class AndroidTransactionProfiler implements ITransactionProfiler {
new ArrayDeque<>();
private final @NotNull Map<String, ProfileMeasurement> measurementsMap = new HashMap<>();

private @Nullable ITransaction currentTransaction = null;

public AndroidTransactionProfiler(
final @NotNull Context context,
final @NotNull SentryAndroidOptions sentryAndroidOptions,
Expand Down Expand Up @@ -140,7 +143,13 @@ private void init() {

@Override
public synchronized void onTransactionStart(final @NotNull ITransaction transaction) {
options.getExecutorService().submit(() -> onTransactionStartSafe(transaction));
try {
options.getExecutorService().submit(() -> onTransactionStartSafe(transaction));
} catch (Throwable e) {
options
.getLogger()
.log(SentryLevel.ERROR, "Failed to call the executor. Profiling will not be started", e);
}
}

@SuppressLint("NewApi")
Expand Down Expand Up @@ -235,13 +244,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 (Throwable e) {
options
.getLogger()
.log(
SentryLevel.ERROR,
"Failed to call the executor. Profiling will not be automatically finished",
e);
}

transactionStartNanos = SystemClock.elapsedRealtimeNanos();
profileStartCpuMillis = Process.getElapsedCpuTime();
Expand All @@ -265,6 +285,11 @@ public void onFrameMetricCollected(
options.getLogger().log(SentryLevel.ERROR, "Error finishing profiling: ", e);
} catch (InterruptedException e) {
options.getLogger().log(SentryLevel.ERROR, "Error finishing profiling: ", e);
} catch (Throwable e) {
options
.getLogger()
.log(
SentryLevel.ERROR, "Failed to call the executor. Profiling could not be finished", e);
}
return null;
}
Expand Down Expand Up @@ -349,6 +374,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);
Expand Down Expand Up @@ -483,6 +509,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.
*
Expand All @@ -503,4 +542,10 @@ private void putPerformanceCollectionDataInMeasurements(
return null;
}
}

@TestOnly
@Nullable
ITransaction getCurrentTransaction() {
return currentTransaction;
}
}
22 changes: 22 additions & 0 deletions sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -150,8 +151,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())
Expand Down Expand Up @@ -401,4 +405,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<Future<*>?>("scheduledFinish")
assertNull(scheduledJob)

// Calling transaction finish returns null, as the profiler was stopped
val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null)
assertNull(profilingTraceData)
}
}
2 changes: 2 additions & 0 deletions sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,7 @@ class SentryAndroidOptionsTest {
transaction: ITransaction,
performanceCollectionData: List<PerformanceCollectionData>?
): ProfilingTraceData? = null

override fun close() {}
}
}
31 changes: 31 additions & 0 deletions ...tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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.Sentry
import io.sentry.android.core.SentryAndroidOptions
import org.junit.runner.RunWith
import kotlin.test.Test

@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<EmptyActivity>()
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()
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()

}
}
5 changes: 5 additions & 0 deletions sentry/api/sentry.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ public final class io/sentry/DateUtils {

public final class io/sentry/DefaultTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector {
public fun <init> (Lio/sentry/SentryOptions;)V
public fun close ()V
public fun start (Lio/sentry/ITransaction;)V
public fun stop (Lio/sentry/ITransaction;)Ljava/util/List;
}
Expand Down Expand Up @@ -597,6 +598,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
}
Expand Down Expand Up @@ -905,12 +907,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
Expand Down Expand Up @@ -2151,6 +2155,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;
}
Expand Down
31 changes: 28 additions & 3 deletions sentry/src/main/java/io/sentry/DefaultTransactionPerformanceCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,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 (Throwable e) {
options
.getLogger()
.log(
SentryLevel.ERROR,
"Failed to call the executor. Performance collector will not be automatically finished",
e);
}
}
if (!isStarted.getAndSet(true)) {
synchronized (timerLock) {
Expand Down Expand Up @@ -113,4 +122,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;
}
}
}
}
}
15 changes: 15 additions & 0 deletions sentry/src/main/java/io/sentry/Hub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,21 @@ public void close() {
((Closeable) integration).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);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it right to close the current transaction?
Also, should we think about closing any other manual transaction started by the user?
Transactions keep a reference to the old options, and so to the executor service, possibly leading to a crash. In reality all calls are now wrapped in try catch blocks, so it's a non-issue.
It's more to understand if it makes sense to stop the transactions from a end user perspective

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense, especially for Mobile. @adinauer does this seem right for you as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

we are dropping canceling the transaction for the moment, as we have to be careful on the backend use case.
Worst case we will show some errors in the logs, pointing at a possible Sentry.close() previous call.
In another pr we will reevaluate cancelling the current transactions.

stefanosiano marked this conversation as resolved.
Show resolved Hide resolved
}
scope.clear();
});
options.getTransactionProfiler().close();
options.getTransactionPerformanceCollector().close();
options.getExecutorService().close(options.getShutdownTimeoutMillis());

// Close the top-most client
Expand Down
8 changes: 5 additions & 3 deletions sentry/src/main/java/io/sentry/ISentryExecutorService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand All @@ -25,10 +26,11 @@ public interface ISentryExecutorService {
* @return a Future of the Callable
*/
@NotNull
<T> Future<T> submit(final @NotNull Callable<T> callable);
<T> Future<T> submit(final @NotNull Callable<T> 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
Expand Down
3 changes: 3 additions & 0 deletions sentry/src/main/java/io/sentry/ITransactionProfiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@ public interface ITransactionProfiler {
ProfilingTraceData onTransactionFinish(
@NotNull ITransaction transaction,
@Nullable List<PerformanceCollectionData> performanceCollectionData);

/** Cancel the profiler and stops it. Used on SDK close. */
void close();
}
3 changes: 3 additions & 0 deletions sentry/src/main/java/io/sentry/NoOpTransactionPerformanceCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@ public void start(@NotNull ITransaction transaction) {}
public @Nullable List<PerformanceCollectionData> stop(@NotNull ITransaction transaction) {
return null;
}

@Override
public void close() {}
}
3 changes: 3 additions & 0 deletions sentry/src/main/java/io/sentry/NoOpTransactionProfiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,7 @@ public void onTransactionStart(@NotNull ITransaction transaction) {}
@Nullable List<PerformanceCollectionData> performanceCollectionData) {
return null;
}

@Override
public void close() {}
}
Loading