-
-
Notifications
You must be signed in to change notification settings - Fork 363
fix: Race condition in SentryApplication and Swift implementation #5900
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
Conversation
721aa8c
to
fa65d1b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5900 +/- ##
=============================================
- Coverage 86.752% 86.653% -0.099%
=============================================
Files 428 429 +1
Lines 36882 36857 -25
Branches 17412 17396 -16
=============================================
- Hits 31996 31938 -58
- Misses 4839 4873 +34
+ Partials 47 46 -1
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
a115144
to
00a642f
Compare
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2609f7a | 1218.17 ms | 1241.34 ms | 23.17 ms |
ad964ca | 1234.73 ms | 1254.88 ms | 20.15 ms |
c6c1cb7 | 1235.71 ms | 1263.80 ms | 28.08 ms |
5258fb8 | 1207.92 ms | 1234.51 ms | 26.59 ms |
efab7d3 | 1219.98 ms | 1252.12 ms | 32.14 ms |
83bb978 | 1238.33 ms | 1260.04 ms | 21.71 ms |
0ee162c | 1226.90 ms | 1261.72 ms | 34.83 ms |
c6d2354 | 1226.53 ms | 1252.69 ms | 26.16 ms |
2de3f92 | 1207.56 ms | 1234.96 ms | 27.40 ms |
f8029e2 | 1245.16 ms | 1261.32 ms | 16.16 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2609f7a | 23.75 KiB | 867.04 KiB | 843.29 KiB |
ad964ca | 23.75 KiB | 913.17 KiB | 889.42 KiB |
c6c1cb7 | 23.75 KiB | 928.15 KiB | 904.40 KiB |
5258fb8 | 23.75 KiB | 874.45 KiB | 850.70 KiB |
efab7d3 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
83bb978 | 23.75 KiB | 920.64 KiB | 896.89 KiB |
0ee162c | 23.75 KiB | 933.33 KiB | 909.58 KiB |
c6d2354 | 23.75 KiB | 919.70 KiB | 895.95 KiB |
2de3f92 | 23.75 KiB | 919.69 KiB | 895.94 KiB |
f8029e2 | 23.75 KiB | 893.72 KiB | 869.97 KiB |
Previous results on branch: updateApplication
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e2a0e66 | 1198.58 ms | 1231.93 ms | 33.35 ms |
8ce26a2 | 1225.54 ms | 1242.65 ms | 17.10 ms |
ee17551 | 1217.88 ms | 1245.02 ms | 27.14 ms |
621c433 | 1219.33 ms | 1246.92 ms | 27.59 ms |
289850d | 1216.71 ms | 1239.00 ms | 22.29 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e2a0e66 | 23.75 KiB | 928.30 KiB | 904.55 KiB |
8ce26a2 | 23.75 KiB | 928.00 KiB | 904.25 KiB |
ee17551 | 23.75 KiB | 928.53 KiB | 904.78 KiB |
621c433 | 23.75 KiB | 928.56 KiB | 904.81 KiB |
289850d | 23.75 KiB | 928.00 KiB | 904.25 KiB |
0a54d3f
to
fc48fe4
Compare
fc48fe4
to
1cc7e90
Compare
🔥 |
1cc7e90
to
8304f48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress, let some comments
4d434eb
to
b117726
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
3438ca1
to
e417f85
Compare
e417f85
to
c2e34f8
Compare
🚨 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:
|
SentryApplication was doing 2 things. It was wrapping UI/NSApplication methods for unit testing purposes, and it was attempting to provide background thread safe access to
isActive
. I say attempting because it used Notification Center to listen for foreground/background and change a state var, but didn't have any kind of locking. So it was prone to a data race where the main thread would write to it through the notification at the same time a background thread read from it.The other issue was the AppKit version direct called in to NSApplication and made no attempt at being thread safe, so this object had different amounts of safety depending on the platform.
This PR separates these two behaviors, so
SentryApplication
is just the system API wrapping (with some extra post-processing on system APIs) andThreadsafeApplication
is a UIKit only thread safe API. ThisThreadsafeApplication
now uses locks to ensure there isn't a data race. Luckily our code was not relying on the macOS version being thread safe, and now that is just more clear at compile time.#skip-changelog