Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reportFullyDisplayed data race #3926

Merged
merged 5 commits into from
May 10, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Fix a data race when calling reportFullyDisplayed from a background thread by synchronizing the call to the main queue.

💡 Motivation and Context

This came up when enabling the thread sanitizer for the iOS-Swift sample app and navigating to the LoremIpsum screen.

CleanShot 2024-04-30 at 17 36 56@2x

💚 How did you test it?

Unit tests and thread sanitizer in the iOS-Swift sample app.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Fix a data race when calling reportFullyDisplayed from a background
thread by synchronizing the call to the main queue.
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.802%. Comparing base (9537eaf) to head (f503ecc).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3926       +/-   ##
=============================================
- Coverage   90.846%   90.802%   -0.045%     
=============================================
  Files          593       592        -1     
  Lines        46028     45949       -79     
  Branches     16432     16363       -69     
=============================================
- Hits         41815     41723       -92     
- Misses        4034      4045       +11     
- Partials       179       181        +2     
Files Coverage Δ
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
.../Sentry/SentryUIViewControllerPerformanceTracker.m 99.024% <100.000%> (+0.009%) ⬆️
...iewController/SentryTimeToDisplayTrackerTest.swift 100.000% <100.000%> (ø)
Tests/SentryTests/SentryHubTests.swift 98.850% <100.000%> (ø)

... and 19 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9537eaf...f503ecc. Read the comment docs.

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1229.15 ms 1251.33 ms 22.18 ms
Size 21.58 KiB 617.00 KiB 595.42 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f74904f 1229.02 ms 1244.91 ms 15.89 ms
591a01b 1242.69 ms 1259.98 ms 17.29 ms
60dd0f5 1212.24 ms 1240.82 ms 28.58 ms
e1cd9e9 1190.64 ms 1221.90 ms 31.26 ms
5f8ee7a 1249.48 ms 1252.20 ms 2.72 ms
69d8759 1229.88 ms 1240.45 ms 10.57 ms
e89dc54 1207.86 ms 1218.27 ms 10.41 ms
f801098 1225.41 ms 1237.45 ms 12.04 ms
c0b4b71 1218.16 ms 1251.28 ms 33.12 ms
bbcbaff 1216.82 ms 1242.34 ms 25.52 ms

App size

Revision Plain With Sentry Diff
f74904f 21.58 KiB 418.71 KiB 397.12 KiB
591a01b 22.84 KiB 401.67 KiB 378.83 KiB
60dd0f5 20.76 KiB 393.37 KiB 372.61 KiB
e1cd9e9 22.85 KiB 412.95 KiB 390.10 KiB
5f8ee7a 22.85 KiB 411.93 KiB 389.08 KiB
69d8759 20.76 KiB 393.05 KiB 372.29 KiB
e89dc54 22.85 KiB 412.60 KiB 389.75 KiB
f801098 21.58 KiB 614.71 KiB 593.13 KiB
c0b4b71 20.76 KiB 430.98 KiB 410.22 KiB
bbcbaff 22.85 KiB 414.09 KiB 391.24 KiB

@philipphofmann philipphofmann force-pushed the fix/data-rate-report-fully-displayed branch from 62422f8 to d308494 Compare May 10, 2024 06:50
@philipphofmann philipphofmann merged commit 8cbdf25 into main May 10, 2024
67 of 70 checks passed
@philipphofmann philipphofmann deleted the fix/data-rate-report-fully-displayed branch May 10, 2024 07:15
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
Fix a data race when calling reportFullyDisplayed from a background
thread by synchronizing the call to the main queue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants