Skip to content

Commit cbe732f

Browse files
runningcodeclaude
andauthored
fix(android): Break the app start extension lock-ordering deadlock (#6007)
* fix(android): Break the app start extension lock-ordering deadlock AppStartExtension held its own lock while calling finish() on the extended span and on the standalone app.start transaction. Finishing captures the transaction synchronously, which runs PerformanceAndroidEventProcessor, which acquires the processor lock and then calls back into isExtended() and getExtendedEndTime() -- taking the extension lock in the opposite order. Two threads is all it takes: the app calls Sentry.finishExtendedAppStart() (extension lock held, waiting for the processor lock) just as the app start deadline fires on the Sentry timer thread (processor lock held, waiting for the extension lock). Both threads hang; if the app finishes the extended app start on the main thread, that is an ANR. Read the fields under the lock and finish outside it, so the extension never holds its lock across a call that reenters the SDK. Span and transaction finishing are already idempotent, so racing callers are safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * changelog * fix(android): Keep the app start finish paths mutually exclusive Moving finish() out of the extension lock let finishTransaction and finishExtendedAppStart interleave. finishTransaction reads the extended span's finish date to clamp the transaction end, so a span finishing in the gap between that read and transaction.finish() captures the transaction with an end earlier than its own child, and both paths could enter SentryTracer.finish concurrently. Serialize the two with a dedicated finishLock that the re-entrant capture path never acquires, so the clamp and the finish are atomic again without putting the processor lock back into a cycle with the extension lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 814017c commit cbe732f

3 files changed

Lines changed: 109 additions & 5 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
- Prevent duplicated breadcrumbs on tombstone-merged native crash events ([#5888](https://github.com/getsentry/sentry-java/pull/5888))
2020
- Prevent a class of Session Replay deadlocks by confining lifecycle state changes to Android's main thread ([#5965](https://github.com/getsentry/sentry-java/pull/5965))
2121
- Symbolicate tombstone native frames for libraries loaded directly from APKs ([#5992](https://github.com/getsentry/sentry-java/pull/5992))
22+
- Prevent a deadlock between the app start extension and the Android performance event processor ([#6007](https://github.com/getsentry/sentry-java/pull/6007))
2223

2324
### Features
2425

‎sentry-android-core/src/main/java/io/sentry/android/core/AppStartExtension.java‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ public interface ExtendAppStartListener {
3434

3535
private final @NotNull AppStartMetrics metrics;
3636
private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
37+
// Serializes the two finish paths against each other. Deliberately separate from `lock`:
38+
// finishing re-enters the SDK and that re-entrant path takes `lock`, so the finish cannot run
39+
// under `lock` (see finishTransaction). When both are held the order is finishLock, then `lock`.
40+
private final @NotNull AutoClosableReentrantLock finishLock = new AutoClosableReentrantLock();
3741

3842
private @Nullable ExtendAppStartListener extendAppStartListener;
3943
// We hold onto both the span and its transaction because they mean different things and finish
@@ -108,8 +112,12 @@ public void setData(final @NotNull String key, final @Nullable Object value) {
108112

109113
@Override
110114
public void finishExtendedAppStart() {
111-
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
112-
final @Nullable ISpan span = extendedSpan;
115+
try (final @NotNull ISentryLifecycleToken ignoredFinish = finishLock.acquire()) {
116+
final @Nullable ISpan span;
117+
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
118+
span = extendedSpan;
119+
}
120+
// Finishing runs outside `lock`, see the note on finishTransaction.
113121
if (span != null && !span.isFinished()) {
114122
span.finish(SpanStatus.OK);
115123
}
@@ -145,10 +153,19 @@ public boolean isExtended() {
145153
}
146154

147155
public void finishTransaction(final @NotNull SentryDate endTimestamp) {
148-
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
149-
final @Nullable ITransaction transaction = extendedTransaction;
156+
// Finishing has to run outside `lock`: it captures the transaction synchronously, which runs
157+
// PerformanceAndroidEventProcessor, which calls back into isExtended()/getExtendedEndTime()
158+
// while holding its own lock. Holding `lock` across the call would let the two be taken in
159+
// opposite orders and deadlock. finishLock still serializes this against
160+
// finishExtendedAppStart, so the end-time clamp below and the finish stay atomic.
161+
try (final @NotNull ISentryLifecycleToken ignoredFinish = finishLock.acquire()) {
162+
final @Nullable ITransaction transaction;
163+
final @Nullable ISpan span;
164+
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
165+
transaction = extendedTransaction;
166+
span = extendedSpan;
167+
}
150168
if (transaction != null && !transaction.isFinished()) {
151-
final @Nullable ISpan span = extendedSpan;
152169
final @Nullable SentryDate spanEnd = span == null ? null : span.getFinishDate();
153170
final @NotNull SentryDate end =
154171
spanEnd != null && spanEnd.isAfter(endTimestamp) ? spanEnd : endTimestamp;

‎sentry-android-core/src/test/java/io/sentry/android/core/AppStartExtensionTest.kt‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import io.sentry.SentryLongDate
88
import io.sentry.SentryNanotimeDate
99
import io.sentry.SpanStatus
1010
import io.sentry.android.core.performance.AppStartMetrics
11+
import java.util.concurrent.CountDownLatch
12+
import java.util.concurrent.TimeUnit
13+
import java.util.concurrent.atomic.AtomicBoolean
1114
import java.util.concurrent.atomic.AtomicInteger
1215
import kotlin.test.Test
1316
import kotlin.test.assertEquals
@@ -17,6 +20,7 @@ import kotlin.test.assertSame
1720
import kotlin.test.assertTrue
1821
import org.junit.runner.RunWith
1922
import org.mockito.kotlin.any
23+
import org.mockito.kotlin.doAnswer
2024
import org.mockito.kotlin.mock
2125
import org.mockito.kotlin.never
2226
import org.mockito.kotlin.verify
@@ -34,6 +38,26 @@ class AppStartExtensionTest {
3438
return AppStartExtension(metrics)
3539
}
3640

41+
private class ReentrantCall(val run: () -> Unit, val succeeded: AtomicBoolean)
42+
43+
/**
44+
* Runs [call] on another thread and records whether it completed while the caller is still inside
45+
* the stubbed method. Used to prove a lock is not held across that call.
46+
*/
47+
private fun reentrantCallDuring(call: () -> Unit): ReentrantCall {
48+
val succeeded = AtomicBoolean(false)
49+
val done = CountDownLatch(1)
50+
val run = {
51+
Thread {
52+
call()
53+
done.countDown()
54+
}
55+
.start()
56+
succeeded.set(done.await(2, TimeUnit.SECONDS))
57+
}
58+
return ReentrantCall(run, succeeded)
59+
}
60+
3761
/** Simulates the integration's listener: hands a transaction + span back to the extension. */
3862
private fun AppStartExtension.registerHandOver(
3963
txn: ITransaction = mock(),
@@ -123,6 +147,68 @@ class AppStartExtensionTest {
123147
verify(span, never()).finish(any<SpanStatus>())
124148
}
125149

150+
@Test
151+
fun `finishExtendedAppStart releases the lock before finishing the span`() {
152+
val ext = extension(windowOpen = true)
153+
val span = mock<ISpan>()
154+
ext.registerHandOver(span = span)
155+
ext.extendAppStart()
156+
157+
// Finishing the real span captures the transaction synchronously, which runs
158+
// PerformanceAndroidEventProcessor, which calls back into the extension while holding its own
159+
// lock. Holding this lock across the finish lets the two be taken in opposite orders and
160+
// deadlock, so require another thread to get through the extension lock during the finish.
161+
val reentered = reentrantCallDuring { ext.isExtended }
162+
doAnswer { reentered.run() }.whenever(span).finish(any<SpanStatus>())
163+
164+
ext.finishExtendedAppStart()
165+
166+
assertTrue(reentered.succeeded.get(), "extension lock was held while finishing the span")
167+
}
168+
169+
@Test
170+
fun `finishTransaction releases the lock before finishing the transaction`() {
171+
val ext = extension(windowOpen = true)
172+
val txn = mock<ITransaction>()
173+
ext.registerHandOver(txn = txn)
174+
ext.extendAppStart()
175+
176+
val reentered = reentrantCallDuring { ext.isExtended }
177+
doAnswer { reentered.run() }.whenever(txn).finish(any<SpanStatus>(), any())
178+
179+
ext.finishTransaction(SentryNanotimeDate())
180+
181+
assertTrue(reentered.succeeded.get(), "extension lock was held while finishing the transaction")
182+
}
183+
184+
@Test
185+
fun `finishTransaction and finishExtendedAppStart do not interleave`() {
186+
val ext = extension(windowOpen = true)
187+
val txn = mock<ITransaction>()
188+
val span = mock<ISpan>()
189+
ext.registerHandOver(txn = txn, span = span)
190+
ext.extendAppStart()
191+
192+
// The end-time clamp in finishTransaction reads the span's finish date and then finishes the
193+
// transaction. If finishExtendedAppStart can finish the span in between, the transaction is
194+
// captured with an end earlier than its own child span.
195+
val spanFinished = AtomicBoolean(false)
196+
val interleaved = AtomicBoolean(false)
197+
doAnswer { spanFinished.set(true) }.whenever(span).finish(any<SpanStatus>())
198+
doAnswer {
199+
val other = Thread { ext.finishExtendedAppStart() }
200+
other.start()
201+
other.join(1_000)
202+
interleaved.set(spanFinished.get())
203+
}
204+
.whenever(txn)
205+
.finish(any<SpanStatus>(), any())
206+
207+
ext.finishTransaction(SentryNanotimeDate())
208+
209+
assertFalse(interleaved.get(), "the extended span was finished mid-finishTransaction")
210+
}
211+
126212
@Test
127213
fun `isActive reflects the transaction state`() {
128214
val ext = extension(windowOpen = true)

0 commit comments

Comments
 (0)