fix: Always start SDK on the main thread (#3291)

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 Co-authored-by: Jim Boulter <[email protected]> Co-authored-by: Philipp Hofmann <[email protected]> Co-authored-by: Andrew McKnight <[email protected]> Fixes GH-3280
getsentry · Oct 11, 2023 · 2401cbd · 2401cbd
1 parent ac614f1
commit 2401cbd
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- Always start SDK on the main thread (#3291)
 - App hang with race condition for tick counter (#3290)
 - Remove "duplicate library" warning (#3312)
 - Remove unnecessary build settings (#3325)

diff --git a/Sources/Sentry/Public/SentrySDK.h b/Sources/Sentry/Public/SentrySDK.h
@@ -27,12 +27,18 @@ SENTRY_NO_INIT
 /**
  * Inits and configures Sentry (SentryHub, SentryClient) and sets up all integrations. Make sure to
  * set a valid DSN.
+ *
+ * @discussion Call this method on the main thread. When calling it from a background thread, the
+ * SDK starts on the main thread async.
  */
 + (void)startWithOptions:(SentryOptions *)options NS_SWIFT_NAME(start(options:));
 
 /**
  * Inits and configures Sentry (SentryHub, SentryClient) and sets up all integrations. Make sure to
  * set a valid DSN.
+ *
+ * @discussion Call this method on the main thread. When calling it from a background thread, the
+ * SDK starts on the main thread async.
  */
 + (void)startWithConfigureOptions:(void (^)(SentryOptions *options))configureOptions
     NS_SWIFT_NAME(start(configureOptions:));

diff --git a/Sources/Sentry/SentryReachability.m b/Sources/Sentry/SentryReachability.m
@@ -98,6 +98,13 @@
     }
 }
 
+void
+SentryConnectivityReset(void)
+{
+    [sentry_reachability_change_blocks removeAllObjects];
+    sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
+}
+
 @implementation SentryReachability
 
 + (void)initialize
@@ -109,9 +116,8 @@ + (void)initialize
 
 - (void)dealloc
 {
-    for (id<SentryReachabilityObserver> observer in sentry_reachability_change_blocks.allKeys) {
-        [self removeObserver:observer];
-    }
+    sentry_reachability_change_blocks = [NSMutableDictionary dictionary];
+    [self removeReachabilityNotification];
 }
 
 - (void)addObserver:(id<SentryReachabilityObserver>)observer
@@ -147,6 +153,11 @@ - (void)removeObserver:(id<SentryReachabilityObserver>)observer
         return;
     }
 
+    [self removeReachabilityNotification];
+}
+
+- (void)removeReachabilityNotification
+{
     sentry_current_reachability_state = kSCNetworkReachabilityFlagsUninitialized;
 
     if (_sentry_reachability_ref == nil) {

diff --git a/Sources/Sentry/SentrySDK.m b/Sources/Sentry/SentrySDK.m
@@ -8,12 +8,14 @@
 #import "SentryCrash.h"
 #import "SentryCrashWrapper.h"
 #import "SentryDependencyContainer.h"
+#import "SentryDispatchQueueWrapper.h"
 #import "SentryFileManager.h"
 #import "SentryHub+Private.h"
 #import "SentryLog.h"
 #import "SentryMeta.h"
 #import "SentryOptions+Private.h"
 #import "SentryScope.h"
+#import "SentryThreadWrapper.h"
 #import "SentryUIDeviceWrapper.h"
 
 @interface
@@ -135,27 +137,32 @@ + (void)setStartInvocations:(NSUInteger)value
 
 + (void)startWithOptions:(SentryOptions *)options
 {
-    startInvocations++;
+    // We accept the tradeoff that the SDK might not be fully initialized directly after
+    // initializing it on a background thread because scheduling the init synchronously on the main
+    // thread could lead to deadlocks.
+    [SentryThreadWrapper onMainThread:^{
+        startInvocations++;
 
-    [SentryLog configure:options.debug diagnosticLevel:options.diagnosticLevel];
+        [SentryLog configure:options.debug diagnosticLevel:options.diagnosticLevel];
 
-    SentryClient *newClient = [[SentryClient alloc] initWithOptions:options];
-    [newClient.fileManager moveAppStateToPreviousAppState];
-    [newClient.fileManager moveBreadcrumbsToPreviousBreadcrumbs];
+        SentryClient *newClient = [[SentryClient alloc] initWithOptions:options];
+        [newClient.fileManager moveAppStateToPreviousAppState];
+        [newClient.fileManager moveBreadcrumbsToPreviousBreadcrumbs];
 
-    SentryScope *scope
-        = options.initialScope([[SentryScope alloc] initWithMaxBreadcrumbs:options.maxBreadcrumbs]);
-    // The Hub needs to be initialized with a client so that closing a session
-    // can happen.
-    [SentrySDK setCurrentHub:[[SentryHub alloc] initWithClient:newClient andScope:scope]];
-    SENTRY_LOG_DEBUG(@"SDK initialized! Version: %@", SentryMeta.versionString);
-    [SentrySDK installIntegrations];
+        SentryScope *scope = options.initialScope(
+            [[SentryScope alloc] initWithMaxBreadcrumbs:options.maxBreadcrumbs]);
+        // The Hub needs to be initialized with a client so that closing a session
+        // can happen.
+        [SentrySDK setCurrentHub:[[SentryHub alloc] initWithClient:newClient andScope:scope]];
+        SENTRY_LOG_DEBUG(@"SDK initialized! Version: %@", SentryMeta.versionString);
+        [SentrySDK installIntegrations];
 
-    [SentryCrashWrapper.sharedInstance startBinaryImageCache];
-    [SentryDependencyContainer.sharedInstance.binaryImageCache start];
+        [SentryCrashWrapper.sharedInstance startBinaryImageCache];
+        [SentryDependencyContainer.sharedInstance.binaryImageCache start];
 #if TARGET_OS_IOS
-    [SentryDependencyContainer.sharedInstance.uiDeviceWrapper start];
+        [SentryDependencyContainer.sharedInstance.uiDeviceWrapper start];
 #endif
+    }];
 }
 
 + (void)startWithConfigureOptions:(void (^)(SentryOptions *options))configureOptions

diff --git a/Tests/SentryTests/Networking/SentryReachabilityTests.m b/Tests/SentryTests/Networking/SentryReachabilityTests.m
@@ -2,6 +2,8 @@
 #import "SentryReachability.h"
 #import <XCTest/XCTest.h>
 
+void SentryConnectivityReset(void);
+
 @interface TestSentryReachabilityObserver : NSObject <SentryReachabilityObserver>
 @end
 @implementation TestSentryReachabilityObserver
@@ -22,6 +24,7 @@ - (void)setUp
 - (void)tearDown
 {
     self.reachability = nil;
+    SentryConnectivityReset();
 }
 
 - (void)testConnectivityRepresentations

diff --git a/Tests/SentryTests/SentrySDKTests.swift b/Tests/SentryTests/SentrySDKTests.swift
@@ -653,7 +653,25 @@ class SentrySDKTests: XCTestCase {
 
         XCTAssertEqual(flushTimeout, transport.flushInvocations.first)
     }
-
+
+    func testStartOnTheMainThread() {
+
+        let expectation = expectation(description: "MainThreadTestIntegration install called")
+        MainThreadTestIntegration.expectation = expectation
+
+        DispatchQueue.global(qos: .background).async {
+            SentrySDK.start { options in
+                options.integrations = [ NSStringFromClass(MainThreadTestIntegration.self) ]
+            }
+        }
+
+        wait(for: [expectation], timeout: 1.0)
+
+        let mainThreadIntegration = SentrySDK.currentHub().installedIntegrations().first { integration in integration is MainThreadTestIntegration } as? MainThreadTestIntegration
+        XCTAssertEqual(mainThreadIntegration?.installedInTheMainThread, true, "SDK is not being initialized in the main thread")
+
+    }
+
 #if SENTRY_HAS_UIKIT
 
     func testSetAppStartMeasurementConcurrently() {
@@ -773,3 +791,17 @@ class SentrySDKTests: XCTestCase {
         fixture.currentDate.setDate(date: SentryDependencyContainer.sharedInstance().dateProvider.date().addingTimeInterval(bySeconds))
     }
 }
+
+public class MainThreadTestIntegration: NSObject, SentryIntegrationProtocol {
+
+    static var expectation: XCTestExpectation?
+
+    public var installedInTheMainThread = false
+
+    public func install(with options: Options) -> Bool {
+        installedInTheMainThread = Thread.isMainThread
+        MainThreadTestIntegration.expectation?.fulfill()
+        MainThreadTestIntegration.expectation = nil
+        return true
+    }
+}
diff --git a/develop-docs/DECISIONS.md b/develop-docs/DECISIONS.md
@@ -127,3 +127,16 @@ This way, if these files are changed, we will be reminded to test the changes wi
 Additionally, two new 'make' commands(test-alamofire, test-homekit) are being added to the project to assist in testing the Sentry SDK in third-party projects.
 
 Related to [GH-2916](https://github.com/getsentry/sentry-cocoa/pull/2916)
+
+## Async SDK init on main thread
+
+Date: October 11th 2023
+Contributors: @philipphofmann, @brustolin
+
+We decided to initialize the SDK on the main thread async when being initialized from a background thread.
+We accept the tradeoff that the SDK might not be fully initialized directly after initializing it on a background
+thread because scheduling the init synchronously on the main thread could lead to deadlocks, such as https://github.com/getsentry/sentry-cocoa/issues/3277.
+
+Related links:
+
+- https://github.com/getsentry/sentry-cocoa/pull/3291