fix: avoid task lock during breadcrumbs - #8497
Draft
philprime wants to merge 3 commits into
Draft
Conversation
Keep Session Replay response processing outside the URLSession task monitor and avoid reacquiring that monitor while adding network-detail breadcrumbs. This prevents synchronous cancellation from waiting on optional replay enrichment. Fixes #8355
Member
Author
|
@sentry review |
📲 Install BuildsiOS
|
Publish request and response details through a nonblocking internal state mutex and serialize consistent snapshots. Skip optional replay enrichment under contention rather than blocking URLSession callbacks.
Contributor
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
Member
Author
|
@sentry review |
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1b1997e | 1222.20 ms | 1262.02 ms | 39.83 ms |
| fc5cefc | 1222.44 ms | 1254.20 ms | 31.77 ms |
| 92bcc8f | 1233.43 ms | 1270.20 ms | 36.77 ms |
| 91f06a6 | 1213.64 ms | 1259.58 ms | 45.93 ms |
| c2a01c0 | 1218.69 ms | 1250.67 ms | 31.98 ms |
| 2509eff | 1219.50 ms | 1253.57 ms | 34.07 ms |
| a781a3d | 1210.98 ms | 1244.31 ms | 33.33 ms |
| f213b0b | 1220.62 ms | 1251.59 ms | 30.97 ms |
| 119dc37 | 1217.64 ms | 1250.44 ms | 32.80 ms |
| 7814719 | 1220.98 ms | 1254.14 ms | 33.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 1b1997e | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| fc5cefc | 24.14 KiB | 1.17 MiB | 1.14 MiB |
| 92bcc8f | 24.14 KiB | 1.15 MiB | 1.13 MiB |
| 91f06a6 | 24.14 KiB | 1.24 MiB | 1.21 MiB |
| c2a01c0 | 24.14 KiB | 1.23 MiB | 1.21 MiB |
| 2509eff | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| a781a3d | 24.14 KiB | 1.16 MiB | 1.13 MiB |
| f213b0b | 24.14 KiB | 1.17 MiB | 1.14 MiB |
| 119dc37 | 24.14 KiB | 1.16 MiB | 1.13 MiB |
| 7814719 | 24.14 KiB | 1.15 MiB | 1.13 MiB |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Avoid holding or reacquiring the
NSURLSessionTaskmonitor while Session Replay processes network response details and attaches them to a network breadcrumb.captureResponseDetails:now uses@synchronized(task)only to retrieve the associatedSentryReplayNetworkDetails. Header access and body parsing happen after releasing the monitor.addBreadcrumbForSessionTask:no longer reacquires the task monitor to read that associated object.SentryReplayNetworkDetailspublishes completed request and response values through a nonblocking internal state mutex. Serialization copies one consistent snapshot before building its dictionary. If optional replay state is busy, the SDK skips that publication or snapshot rather than blocking a URLSession callback or main-thread cancellation.Deterministic regression tests coordinate two threads on the same task monitor and concurrently update and serialize replay details. They cover the main-thread
setState:breadcrumb path, the response-processing critical section, and consistent network-detail snapshots.💡 Motivation and Context
NSURLSessionTask.cancelcan invoke Sentry's swizzledsetState:synchronously on the main thread. The tracker then adds a breadcrumb and previously waited on@synchronized(sessionTask)if a URLSession worker was capturing Session Replay response details for the same task. Because response header and body processing happened inside that monitor, this contention could keep the main thread blocked long enough to trigger a fatal app hang.The reproduced sequence is:
sequenceDiagram participant Worker as URLSession worker participant Monitor as NSURLSessionTask monitor participant Main as Main thread Worker->>Monitor: captureResponseDetails acquires monitor Worker->>Worker: Process response headers and body Main->>Main: NSURLSessionTask.cancel Main->>Main: Sentry swizzled setState Main->>Monitor: addBreadcrumbForSessionTask waits Worker-->>Monitor: Release monitor Monitor-->>Main: Continue cancellationThis evidence establishes lock contention causing an app hang. It does not by itself establish a permanent deadlock.
Fixes #8355
Sentry issue: https://sentry.sentry.io/issues/6827836853/
💚 How did you test it?
The breadcrumb contention test failed before the fix because
setState:took about 1.07 seconds while another thread held the task monitor. The response-processing test also failed because another thread could not acquire the task monitor within 0.5 seconds while response headers were read. The concurrent snapshot test produced 96,803 mismatched status and response-size pairs before internal state synchronization and zero afterward.After the fix:
make build-ios FOR_AGENTS=truesucceeded.make analyzesucceeded.make formatcompleted successfully.📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.