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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5300 +/- ##
=============================================
+ Coverage 86.153% 86.274% +0.121%
=============================================
Files 401 402 +1
Lines 34773 34905 +132
Branches 15033 15132 +99
=============================================
+ Hits 29958 30114 +156
+ Misses 4774 4746 -28
- Partials 41 45 +4
... and 26 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 |
|---|---|---|---|
| b714cb9 | 1224.71 ms | 1238.04 ms | 13.33 ms |
| 5cfc768 | 1220.74 ms | 1245.06 ms | 24.32 ms |
| 4e3915a | 1230.02 ms | 1258.90 ms | 28.88 ms |
| 35c962f | 1207.61 ms | 1235.90 ms | 28.29 ms |
| 8ea5293 | 1242.70 ms | 1262.25 ms | 19.55 ms |
| d38165b | 1211.41 ms | 1242.49 ms | 31.08 ms |
| 8047b99 | 1226.37 ms | 1246.63 ms | 20.26 ms |
| b13e93a | 1236.24 ms | 1247.33 ms | 11.08 ms |
| 55f739c | 1226.06 ms | 1248.78 ms | 22.71 ms |
| 2691350 | 1224.92 ms | 1255.82 ms | 30.90 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b714cb9 | 23.75 KiB | 858.69 KiB | 834.93 KiB |
| 5cfc768 | 23.75 KiB | 850.73 KiB | 826.98 KiB |
| 4e3915a | 23.75 KiB | 858.69 KiB | 834.94 KiB |
| 35c962f | 23.75 KiB | 854.77 KiB | 831.02 KiB |
| 8ea5293 | 23.75 KiB | 852.24 KiB | 828.49 KiB |
| d38165b | 23.75 KiB | 855.37 KiB | 831.62 KiB |
| 8047b99 | 23.75 KiB | 855.37 KiB | 831.62 KiB |
| b13e93a | 23.75 KiB | 855.37 KiB | 831.62 KiB |
| 55f739c | 23.75 KiB | 858.73 KiB | 834.98 KiB |
| 2691350 | 23.75 KiB | 850.73 KiB | 826.98 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. |
|
Thanks for the review @armcknight. I'm sorry about the hassle. The actual problem is in the test |
📜 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.