fix: Race condition for app launch profiling #5300
Conversation
When having app launch profiling and TTFD enabled, a race condition when reading alwaysWaitForFullDisplay could occur. This is fixed now by reading if TTFD is enabled from the options instead of the UIViewControllerPerformanceTracker.
philipphofmann
requested review from
armcknight,
philprime,
noahsmartin,
trevor-e,
itaybre and
NicoHinderling
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5300 +/- ##
=============================================
+ Coverage 92.773% 92.811% +0.037%
=============================================
Files 690 691 +1
Lines 86560 86710 +150
Branches 30009 30117 +108
=============================================
+ Hits 80305 80477 +172
+ Misses 6158 6139 -19
+ Partials 97 94 -3
... and 28 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| adcc7d8 | 1225.90 ms | 1245.08 ms | 19.18 ms |
| dacf894 | 1232.32 ms | 1236.34 ms | 4.02 ms |
| 3437454 | 1254.04 ms | 1259.50 ms | 5.46 ms |
| 12e65d0 | 1232.17 ms | 1265.39 ms | 33.22 ms |
| 9f0d9e0 | 1206.55 ms | 1219.41 ms | 12.86 ms |
| 963b49c | 1244.94 ms | 1256.55 ms | 11.61 ms |
| 2283356 | 1228.77 ms | 1248.47 ms | 19.70 ms |
| e5dcbd5 | 1223.47 ms | 1243.90 ms | 20.43 ms |
| d3abae0 | 1200.36 ms | 1224.22 ms | 23.87 ms |
| 83887af | 1196.94 ms | 1206.82 ms | 9.88 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| adcc7d8 | 20.76 KiB | 426.15 KiB | 405.39 KiB |
| dacf894 | 20.76 KiB | 426.34 KiB | 405.58 KiB |
| 3437454 | 22.85 KiB | 408.87 KiB | 386.02 KiB |
| 12e65d0 | 22.30 KiB | 756.53 KiB | 734.23 KiB |
| 9f0d9e0 | 21.58 KiB | 424.28 KiB | 402.70 KiB |
| 963b49c | 22.30 KiB | 749.83 KiB | 727.53 KiB |
| 2283356 | 23.76 KiB | 820.30 KiB | 796.54 KiB |
| e5dcbd5 | 22.85 KiB | 414.09 KiB | 391.24 KiB |
| d3abae0 | 20.76 KiB | 434.92 KiB | 414.16 KiB |
| 83887af | 21.58 KiB | 419.64 KiB | 398.06 KiB |
Previous results on branch: fix/race-condition-app-launch-profiling
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 14eb066 | 1237.76 ms | 1255.47 ms | 17.71 ms |
| 3311429 | 1234.13 ms | 1259.98 ms | 25.85 ms |
| ea59633 | 1230.90 ms | 1249.35 ms | 18.45 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 14eb066 | 23.76 KiB | 821.39 KiB | 797.63 KiB |
| 3311429 | 23.76 KiB | 821.38 KiB | 797.62 KiB |
| ea59633 | 23.76 KiB | 821.29 KiB | 797.53 KiB |
|
All the UI tests are failing. I need to double check if I actually broke something. |
There was a problem hiding this comment.
Thanks for catching this @philipphofmann . We just need to check a few more options for it to be complete.
| @@ -31,6 +31,27 @@ extension SentryAppLaunchProfilingTests { | |||
| XCTAssertNil(sentry_launchTracer) | |||
| } | |||
|
|
|||
| // TTFD only works when UIKit is available, so we only test this on iOS. | |||
There was a problem hiding this comment.
we should just change the #if os(iOS) || os(macOS) || targetEnvironment(macCatalyst) to this at the beginning/end of this file. copypasta on my part!
| SentryUIViewControllerPerformanceTracker *performanceTracker = | ||
| [SentryDependencyContainer.sharedInstance uiViewControllerPerformanceTracker]; | ||
| if (profileIsCorrelatedToTrace && performanceTracker.alwaysWaitForFullDisplay) { | ||
|
|
||
| if (profileIsCorrelatedToTrace && options.enableTimeToFullDisplayTracing) { |
There was a problem hiding this comment.
I think we'll also need to check a few other options, like options.enableUIViewControllerTracing, since you could theoretically misconfigure the SDK like:
options.enableUIViewControllerTracing = false
options.enableTimeToFullDisplayTracing = true
Checking SentryUIViewControllerPerformanceTracker.alwaysWaitForFullDisplay would have implicitly included all the option combination validation. I don't know what other options would be applicable, maybe enableAutoPerformanceTracing and tracesSampleRate/tracesSampler?
There was a problem hiding this comment.
This is actually why the UI tests are failing now. Since we automatically configure enableUIViewControllerTracing to true unless the launch flag "--disable-time-to-full-display-tracing" is set in SentrySDKWrapper/SentrySDKOverrides, changing this now caused the conditional branch to be taken here where it was not before.
|
Thanks for the review @armcknight. Due to the hotfix and some unexpected immediate family member care leave yesterday, I was unable to include your feedback yet. As I'm on PTO the whole next week, this PR will lie around for a bit. If this is important, feel free to pick it up and as @philprime for another review. I can also tackle it once I'm back. |
📜 Description
When having app launch profiling and TTFD enabled, a race condition when reading alwaysWaitForFullDisplay could occur. This is fixed now by reading if TTFD is enabled from the options instead of the UIViewControllerPerformanceTracker.
💡 Motivation and Context
Running the tests with the thread sanitizer enabled surfaced this data race.
The problem is that the
SentryPerformanceTrackingIntegrationwritesalwaysWaitForFullDisplayon the main thread heresentry-cocoa/Sources/Sentry/SentryPerformanceTrackingIntegration.m
Lines 49 to 51 in 922a62e
While the SentryProfiler reads the same value on a BG thread here
sentry-cocoa/Sources/Sentry/SentryProfiler.mm
Lines 91 to 119 in 922a62e
💚 How did you test it?
Running the tests again with the thread sanitizer enabled didn't surface this data race anymore. Ideally, we would write an integration test for this, but that is quite complicated. Enabling the thread sanitizer again in CI will surface such issues again.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.