fix: Always start SDK on the main thread #3291

brustolin · 2023-09-19T15:04:32Z

📜 Description

We always assume the SDK would be initialized in the main thread during app initialization and we design the SDK around this idea (although not documented). This change ensures the initialization of the SDK in the main thread which will avoid other issues like #3280

💡 Motivation and Context

closes #3280

💚 How did you test it?

Unit test

📝 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

This reverts commit b11c6c1.

github-actions · 2023-09-19T15:18:13Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1241.47 ms	1266.67 ms	25.21 ms
Size	22.85 KiB	408.85 KiB	386.00 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`6bc5ae5`	1207.23 ms	1216.66 ms	9.43 ms
`0d32275`	1245.80 ms	1267.80 ms	22.00 ms
`7bc3c0d`	1261.16 ms	1278.38 ms	17.22 ms
`9fa25c2`	1211.88 ms	1228.36 ms	16.48 ms
`df2986b`	1227.45 ms	1244.35 ms	16.90 ms
`32e64d1`	6272.19 ms	6299.50 ms	27.31 ms
`b6ba04e`	1230.48 ms	1253.20 ms	22.72 ms
`c78683b`	1246.71 ms	1258.70 ms	11.99 ms
`4f660b4`	1225.44 ms	1230.18 ms	4.75 ms
`c9724f9`	1199.38 ms	1229.54 ms	30.16 ms

App size

Revision	Plain	With Sentry	Diff
`6bc5ae5`	20.76 KiB	401.39 KiB	380.63 KiB
`0d32275`	22.84 KiB	403.14 KiB	380.29 KiB
`7bc3c0d`	20.76 KiB	427.36 KiB	406.59 KiB
`9fa25c2`	22.85 KiB	407.44 KiB	384.59 KiB
`df2986b`	22.85 KiB	406.89 KiB	384.04 KiB
`32e64d1`	20.76 KiB	433.18 KiB	412.42 KiB
`b6ba04e`	20.76 KiB	414.44 KiB	393.68 KiB
`c78683b`	20.76 KiB	435.24 KiB	414.48 KiB
`4f660b4`	22.85 KiB	408.84 KiB	385.99 KiB
`c9724f9`	20.76 KiB	427.66 KiB	406.90 KiB

Previous results on branch: fix/access-UISession-main-thread

Startup times

Revision	Plain	With Sentry	Diff
`b29b31d`	1257.30 ms	1279.60 ms	22.30 ms
`3f07d67`	1254.96 ms	1256.14 ms	1.18 ms
`349516b`	1217.77 ms	1231.30 ms	13.53 ms
`61894d3`	1226.06 ms	1245.32 ms	19.26 ms
`4dbdaed`	1226.33 ms	1236.74 ms	10.41 ms
`db894fd`	1270.41 ms	1277.38 ms	6.97 ms
`67551be`	1237.31 ms	1244.28 ms	6.97 ms
`896e7eb`	1224.24 ms	1251.42 ms	27.18 ms
`3eb83a8`	1232.88 ms	1244.90 ms	12.02 ms
`21f2b5a`	1199.76 ms	1217.48 ms	17.72 ms

App size

Revision	Plain	With Sentry	Diff
`b29b31d`	22.85 KiB	408.84 KiB	386.00 KiB
`3f07d67`	22.85 KiB	407.75 KiB	384.90 KiB
`349516b`	22.85 KiB	408.84 KiB	385.99 KiB
`61894d3`	22.85 KiB	408.92 KiB	386.07 KiB
`4dbdaed`	22.85 KiB	408.85 KiB	386.00 KiB
`db894fd`	22.85 KiB	408.85 KiB	386.00 KiB
`67551be`	22.85 KiB	408.94 KiB	386.09 KiB
`896e7eb`	22.85 KiB	408.84 KiB	385.99 KiB
`3eb83a8`	22.85 KiB	408.93 KiB	386.08 KiB
`21f2b5a`	22.85 KiB	407.79 KiB	384.94 KiB

armcknight

A couple questions/requests for this one

Sources/SentryCrash/Recording/Tools/SentryCrashObjCApple.h

Tests/SentryTests/SentryOptionsTest.m

SentryTestUtils/MainThreadTestIntegration.swift

Sources/Sentry/SentrySDK.m

Tests/SentryTests/SentryOptionsTest.m

codecov · 2023-09-20T11:30:15Z

Codecov Report

Merging #3291 (0c88c23) into main (ac614f1) will increase coverage by 0.039%.
The diff coverage is 97.826%.

@@              Coverage Diff              @@
##              main     #3291       +/-   ##
=============================================
+ Coverage   89.269%   89.308%   +0.039%     
=============================================
  Files          501       501               
  Lines        54078     54258      +180     
  Branches     19203     19496      +293     
=============================================
+ Hits         48275     48457      +182     
+ Misses        4949      4946        -3     
- Partials       854       855        +1

Files	Coverage Δ
Sources/Sentry/SentryReachability.m	`97.333% <100.000%> (+0.231%)`	⬆️
Sources/Sentry/SentrySDK.m	`89.655% <100.000%> (+0.089%)`	⬆️
Sources/Sentry/SentryThreadWrapper.m	`100.000% <100.000%> (ø)`
...s/SentryTests/Networking/SentryReachabilityTests.m	`100.000% <100.000%> (ø)`
Tests/SentryTests/SentrySDKTests.swift	`97.319% <92.857%> (-0.094%)`	⬇️

... and 65 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 ac614f1...0c88c23. Read the comment docs.

username0x0a · 2023-09-20T17:17:29Z

Sources/Sentry/SentrySDK.m

+        [SentryDependencyContainer.sharedInstance.dispatchQueueWrapper
+            dispatchAsyncOnMainQueue:startSDK];


I think I've seen another dispatch invocation ([SentryThreadWrapper onMainThread:]) in the other PR 🤔 I guess this one resolving SentryDependencyContainer.sharedInstance (which visits some sync lock again) should be dropped completely in favour of wrapping in the SentryThreadWrapper type, just so that something so simple as sync/async dispatch to Main thread doesn't have to touch multiple instances around the Sentry scope & be performed with a very clear call path. 👌

Thanks for following along here, the more eyes the better 👀 I guess you're talking about this: https://github.com/getsentry/sentry-cocoa/pull/3295/files#diff-1c7b8db05183d8ca8e29ad4450733675d013a6b4065104f150d37d04017a36beR20

That will always immediately return, as it will either directly execute the logic on the main thread if it's called from that context, or will dispatch asynchronously to the main queue otherwise, so we shouldn't see lock contention. The only issue is that if SDK consumers initialize from a non-main context, they can potentially lose information for events that happen very early in their launch sequence, but that should hopefully be a calculated tradeoff for whomever does that. We are going to recommend initializing on the main thread in our docs to avoid that kind of thing.

We are also working to resolve the lock contention we're seeing in SentryDependencyContainer in a third PR: #3294

Does this all sound correct/acceptable to you?

Looking again, just a note for the future: we should have -[SentryThreadWrapper onMainThread:] use the dispatch queue wrapper, and then use it here as well since it's the same logic. Doesn't have to be in this PR though, it might involve other code/test changes.

…ry/sentry-cocoa into fix/access-UISession-main-thread

armcknight · 2023-09-22T22:35:14Z

The new test is failing in only some runs, will need more attention.

…ry/sentry-cocoa into fix/access-UISession-main-thread

philipphofmann

@brustolin, what's missing to get this merged? I see that the test is commented out. Let's please fix that.

brustolin · 2023-10-03T14:50:49Z

@brustolin, what's missing to get this merged? I see that the test is commented out. Let's please fix that.

CI, very flaky, Im investigating.

brustolin · 2023-10-04T17:24:55Z

Sources/Sentry/SentryReachability.m

-    for (id<SentryReachabilityObserver> observer in sentry_reachability_change_blocks.allKeys) {
-        [self removeObserver:observer];
-    }


This make reachability flaky during tests.

Shouldn't this change be in an extra PR?

brustolin · 2023-10-04T17:25:20Z

@philipphofmann I think this is ready for review!!

philipphofmann

LGTM

CHANGELOG.md

SentryTestUtils/MainThreadTestIntegration.swift

philipphofmann · 2023-10-05T12:56:43Z

Sources/Sentry/SentryReachability.m

-    for (id<SentryReachabilityObserver> observer in sentry_reachability_change_blocks.allKeys) {
-        [self removeObserver:observer];
-    }


Shouldn't this change be in an extra PR?

Replaced macOS OS version retrieval from a deprecated API to a new one

Co-authored-by: Andrew McKnight <[email protected]>

This building settings were causing warning with Xcode 15

Co-authored-by: Jim Boulter <[email protected]>

brustolin · 2023-10-11T08:12:52Z

Sources/Sentry/SentrySDK.m

+    // We have to call the init on the main thread synchronously to guarantee the SDK gets
+    // initialized. Otherwise, users would init the SDK, and calls directly after would have
+    // undefined behavior as the SDK might not be initialized yet.
+    [SentryThreadWrapper onMainThreadSync:^{


@philipphofmann, the first idea was to make it sync. But @armcknight and me agreed that making it sync may lead to deadlocks as we had in #3277.

We are aware of undefined behaviour if the user chooses to start it in a background thread, but that would be a user choice. I believe, the best thing we can do here is to document this very well.

I was also afraid that this might cause deadlocks. We should be fine with the tradeoff if someone inits the SDK on a background thread that it isn't ready immediately before running into a deadlock. I will add some docs.

brustolin added 4 commits September 19, 2023 16:23

Update SentryCrashIntegration.m

b11c6c1

Revert "Update SentryCrashIntegration.m"

23da6cc

This reverts commit b11c6c1.

Update SentrySDK.m

95ab17a

tests

5a9e4ea

brustolin requested review from philipphofmann and armcknight as code owners September 19, 2023 15:04

brustolin and others added 2 commits September 19, 2023 17:05

Update CHANGELOG.md

f0b432f

Format code

4fa8e1f

armcknight reviewed Sep 19, 2023

View reviewed changes

brustolin and others added 3 commits September 20, 2023 09:22

Update Sources/SentryCrash/Recording/Tools/SentryCrashObjCApple.h

6ee2b67

Format code

490ed4f

Apply suggestions from code review

3ffba00

brustolin commented Sep 20, 2023

View reviewed changes

Tests/SentryTests/SentryOptionsTest.m Outdated Show resolved Hide resolved

Format code

af5a3e5

brustolin commented Sep 20, 2023

View reviewed changes

Tests/SentryTests/SentryOptionsTest.m Outdated Show resolved Hide resolved

brustolin and others added 3 commits September 20, 2023 11:25

Update SentrySDK.m

26f8b49

Format code

b87e505

Update SentrySDKTests.swift

673d34c

username0x0a reviewed Sep 20, 2023

View reviewed changes

brustolin added 3 commits September 21, 2023 10:45

Merge branch 'main' into fix/access-UISession-main-thread

9304c1d

Update SentrySDK.m

5791da4

Merge branch 'fix/access-UISession-main-thread' of github.com:getsent…

ddc053b

…ry/sentry-cocoa into fix/access-UISession-main-thread

brustolin and others added 2 commits September 25, 2023 10:02

Merge branch 'main' into fix/access-UISession-main-thread

17e6590

Format code

b96bc4b

armcknight mentioned this pull request Sep 26, 2023

meta: update Brew/Gem dependencies #3307

Merged

brustolin added 2 commits September 28, 2023 13:28

Merge branch 'main' into fix/access-UISession-main-thread

3418390

Update SentrySDKTests.swift

73db556

brustolin and others added 3 commits September 28, 2023 16:30

Update SentrySDK.m

d41ebfc

Merge branch 'fix/access-UISession-main-thread' of github.com:getsent…

60723f7

…ry/sentry-cocoa into fix/access-UISession-main-thread

Format code

3194c1d

philipphofmann reviewed Oct 3, 2023

View reviewed changes

brustolin and others added 3 commits October 4, 2023 15:59

Merge branch 'main' into fix/access-UISession-main-thread

431978a

Update SentryReachability.m

6692ff9

Format code

aedd343

brustolin requested a review from philipphofmann October 4, 2023 17:24

brustolin commented Oct 4, 2023

View reviewed changes

Update SentrySDKTests.swift

cc4d0e3

philipphofmann approved these changes Oct 5, 2023

View reviewed changes

philipphofmann and others added 10 commits October 11, 2023 09:02

Update CHANGELOG.md

f58236a

ref: Replace deprecated function (#3327)

33e5dbe

Replaced macOS OS version retrieval from a deprecated API to a new one

ci: update unit test iOS/Xcode versions (#3283)

9bb1c59

Co-authored-by: Andrew McKnight <[email protected]>

Fix: Remove unnecessaries build settings (#3325)

4815a2f

This building settings were causing warning with Xcode 15

meta: update ruby version and gem/brew bundles (#3324)

4ecd1bb

fix: remove relative component of import (#3332)

86d4247

Co-authored-by: Jim Boulter <[email protected]>

rename replaceOptionIntegrations

ff64ac7

Merge branch 'main' into fix/access-UISession-main-thread

b1292b1

fix changelog

d174abf

Make SDK init synchronous

0c88c23

philipphofmann changed the title ~~fix: Start SDK only in the main thread~~ fix: Always start SDK on the main thread Oct 11, 2023

brustolin commented Oct 11, 2023

View reviewed changes

philipphofmann added 3 commits October 11, 2023 10:57

change back to init async

e875917

remove not needed file

0f5d42c

simplify comment

d2a9730

philipphofmann merged commit 2401cbd into main Oct 11, 2023
62 of 66 checks passed

philipphofmann deleted the fix/access-UISession-main-thread branch October 11, 2023 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Always start SDK on the main thread #3291

fix: Always start SDK on the main thread #3291

brustolin commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: fix/access-UISession-main-thread

Startup times

App size

armcknight left a comment

codecov bot commented Sep 20, 2023 •

edited

Loading

username0x0a Sep 20, 2023

armcknight Sep 20, 2023

armcknight Sep 20, 2023

armcknight commented Sep 22, 2023

philipphofmann left a comment

brustolin commented Oct 3, 2023

brustolin Oct 4, 2023

philipphofmann Oct 5, 2023

brustolin commented Oct 4, 2023

philipphofmann left a comment

philipphofmann Oct 5, 2023

brustolin Oct 11, 2023 •

edited

Loading

philipphofmann Oct 11, 2023

		[SentryDependencyContainer.sharedInstance.dispatchQueueWrapper
		dispatchAsyncOnMainQueue:startSDK];

fix: Always start SDK on the main thread #3291

fix: Always start SDK on the main thread #3291

Conversation

brustolin commented Sep 19, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Sep 19, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: fix/access-UISession-main-thread

Startup times

App size

armcknight left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2023 • edited Loading

Codecov Report

username0x0a Sep 20, 2023

Choose a reason for hiding this comment

armcknight Sep 20, 2023

Choose a reason for hiding this comment

armcknight Sep 20, 2023

Choose a reason for hiding this comment

armcknight commented Sep 22, 2023

philipphofmann left a comment

Choose a reason for hiding this comment

brustolin commented Oct 3, 2023

brustolin Oct 4, 2023

Choose a reason for hiding this comment

philipphofmann Oct 5, 2023

Choose a reason for hiding this comment

brustolin commented Oct 4, 2023

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Oct 5, 2023

Choose a reason for hiding this comment

brustolin Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

philipphofmann Oct 11, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2023 •

edited

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading

brustolin Oct 11, 2023 •

edited

Loading