Skip to content

Commit ff7e433

Browse files
runningcodeclaude
andauthored
docs: Add a concurrency section to AGENTS.md (#6009)
Two production ANRs in the Android SDK came from holding a lock across a call that re-enters the SDK: finishing a span or transaction captures synchronously on the calling thread and runs every event processor, and those processors lock and call back into the component that is still holding its own lock. Write the rule down next to the exception-handling guidance, with the snapshot-then-call pattern, the second-lock escape hatch for compound operations that were only atomic because the lock spanned them, and the volatile and single-read rules. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 43a5c14 commit ff7e433

1 file changed

Lines changed: 48 additions & 0 deletions

File tree

‎AGENTS.md‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,54 @@ Apply that pattern only where a broad catch is genuinely unavoidable — an entr
154154
arbitrary user code or third-party callbacks. Everywhere else, name the exception types. Say in the
155155
PR description why the broad catch is necessary.
156156

157+
### Concurrency
158+
159+
**Never hold a lock across a call that can re-enter the SDK.** Read what you need under the lock,
160+
release it, then make the call.
161+
162+
Finishing a span or transaction, capturing an event, and invoking a user-supplied callback all run
163+
arbitrary SDK code on the calling thread. `SentryTracer.finish()` calls `scopes.captureTransaction(...)`
164+
synchronously, which runs every registered `EventProcessor` — and those processors take their own
165+
locks and call back into SDK components. A lock held across such a call is therefore acquired in the
166+
opposite order from the processor's, which is a deadlock. On Android the blocked thread is usually
167+
the main thread, so it surfaces to users as an ANR rather than a hang.
168+
169+
```java
170+
// Wrong: the lock is held while finish() captures the transaction and runs event processors,
171+
// which take their own lock and then call back into this class.
172+
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
173+
final @Nullable ITransaction transaction = ownedTransaction;
174+
if (transaction != null && !transaction.isFinished()) {
175+
transaction.finish(SpanStatus.OK);
176+
}
177+
}
178+
179+
// Right: read the field under the lock, call finish() outside it.
180+
final @Nullable ITransaction transaction;
181+
try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
182+
transaction = ownedTransaction;
183+
}
184+
if (transaction != null && !transaction.isFinished()) {
185+
transaction.finish(SpanStatus.OK);
186+
}
187+
```
188+
189+
The same applies to logging through `options.getLogger()`, invoking a listener, and any
190+
`ISpan`/`IScopes` call reachable from a component that locks.
191+
192+
Moving the call out can break a compound operation that was atomic only because the lock spanned
193+
it. When that happens, do not widen the original lock again — serialize the callers with a second
194+
lock that the re-entrant path never acquires, and document the order the two are taken in.
195+
196+
Two more rules that catch most of the rest:
197+
198+
- **Mark a field `volatile` when it is written on one thread and read on another without a lock.**
199+
A plain field read is a data race, not merely a stale value.
200+
- **Read mutable shared state once per operation.** Re-reading the same field for several
201+
decisions in one pass lets it change mid-pass, so the results disagree with each other.
202+
203+
Prefer `AutoClosableReentrantLock` over `synchronized`, matching the rest of the codebase.
204+
157205
### Testing Requirements
158206
- Write comprehensive unit tests for new features
159207
- Android modules require both unit tests and instrumented tests where applicable

0 commit comments

Comments
 (0)