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 AndroidProfiler and SentryTracer crashes #2731

Merged
merged 2 commits into from
May 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
### Fixes

- Base64 encode internal Apollo3 Headers ([#2707](https://github.com/getsentry/sentry-java/pull/2707))
- Fix `SentryTracer` crash when scheduling auto-finish of a transaction, but the timer has already been cancelled ([#2731](https://github.com/getsentry/sentry-java/pull/2731))
- Fix `AndroidTransactionProfiler` crash when finishing a profile that happened due to race condition ([#2731](https://github.com/getsentry/sentry-java/pull/2731))

## 6.19.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ public void onFrameMetricCollected(
}

@SuppressLint("NewApi")
private @Nullable ProfilingTraceData onTransactionFinish(
private @Nullable synchronized ProfilingTraceData onTransactionFinish(
stefanosiano marked this conversation as resolved.
Show resolved Hide resolved
final @NotNull ITransaction transaction,
final boolean isTimeout,
final @Nullable List<PerformanceCollectionData> performanceCollectionData) {
Expand Down Expand Up @@ -365,7 +365,10 @@ public void onFrameMetricCollected(
long transactionDurationNanos = transactionEndNanos - transactionStartNanos;

List<ProfilingTransactionData> transactionList = new ArrayList<>(1);
transactionList.add(currentProfilingTransactionData);
final ProfilingTransactionData txData = currentProfilingTransactionData;
if (txData != null) {
transactionList.add(txData);
}
currentProfilingTransactionData = null;
// We clear the counter in case of a timeout
transactionsCounter = 0;
Expand Down
25 changes: 20 additions & 5 deletions sentry/src/main/java/io/sentry/SentryTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,29 @@ public void scheduleFinish() {
new TimerTask() {
@Override
public void run() {
final SpanStatus status = getStatus();
finish((status != null) ? status : SpanStatus.OK);
isFinishTimerRunning.set(false);
finishFromTimer();
}
};

timer.schedule(timerTask, transactionOptions.getIdleTimeout());
try {
timer.schedule(timerTask, transactionOptions.getIdleTimeout());
} catch (Throwable e) {
hub.getOptions()
.getLogger()
.log(SentryLevel.WARNING, "Failed to schedule finish timer", e);
// if we failed to schedule the finish timer for some reason, we finish it here right away
finishFromTimer();
}
}
}
}

private void finishFromTimer() {
final SpanStatus status = getStatus();
finish((status != null) ? status : SpanStatus.OK);
isFinishTimerRunning.set(false);
}

@Override
public @NotNull void forceFinish(@NotNull SpanStatus status, boolean dropIfNoChildren) {
if (isFinished()) {
Expand Down Expand Up @@ -227,7 +239,10 @@ public void finish(
// if it's an idle transaction which has no children, we drop it to save user's quota
hub.getOptions()
.getLogger()
.log(SentryLevel.DEBUG, "Dropping idle transaction because it has no child spans");
.log(
SentryLevel.DEBUG,
"Dropping idle transaction %s because it has no child spans",
name);
return;
}

Expand Down
19 changes: 19 additions & 0 deletions sentry/src/test/java/io/sentry/SentryTracerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1216,4 +1216,23 @@ class SentryTracerTest {
anyOrNull()
)
}

@Test
fun `when timer is cancelled, schedule finish does not crash`() {
val tracer = fixture.getSut(idleTimeout = 50)
tracer.timer!!.cancel()
tracer.scheduleFinish()
}

@Test
fun `when timer is cancelled, schedule finish finishes the transaction immediately`() {
val tracer = fixture.getSut(idleTimeout = 50)
tracer.startChild("load").finish()

tracer.timer!!.cancel()
tracer.scheduleFinish()

assertTrue(tracer.isFinished)
verify(fixture.hub).captureTransaction(any(), anyOrNull(), anyOrNull(), anyOrNull())
}
}