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.

Sign up for GitHub

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: Always start SDK on the main thread #3291

Merged
merged 47 commits into from
Oct 11, 2023
Merged

fix: Always start SDK on the main thread #3291

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
b11c6c1
Update SentryCrashIntegration.m
brustolin Sep 19, 2023
23da6cc
Revert "Update SentryCrashIntegration.m"
brustolin Sep 19, 2023
95ab17a
Update SentrySDK.m
brustolin Sep 19, 2023
5a9e4ea
tests
brustolin Sep 19, 2023
f0b432f
Update CHANGELOG.md
brustolin Sep 19, 2023
4fa8e1f
Format code
getsentry-bot Sep 19, 2023
6ee2b67
Update Sources/SentryCrash/Recording/Tools/SentryCrashObjCApple.h
brustolin Sep 20, 2023
490ed4f
Format code
getsentry-bot Sep 20, 2023
3ffba00
Apply suggestions from code review
brustolin Sep 20, 2023
af5a3e5
Format code
getsentry-bot Sep 20, 2023
26f8b49
Update SentrySDK.m
brustolin Sep 20, 2023
b87e505
Format code
getsentry-bot Sep 20, 2023
673d34c
Update SentrySDKTests.swift
brustolin Sep 20, 2023
9304c1d
Merge branch 'main' into fix/access-UISession-main-thread
brustolin Sep 21, 2023
5791da4
Update SentrySDK.m
brustolin Sep 21, 2023
ddc053b
Merge branch 'fix/access-UISession-main-thread' of github.com:getsent…
brustolin Sep 21, 2023
17e6590
Merge branch 'main' into fix/access-UISession-main-thread
brustolin Sep 25, 2023
b96bc4b
Format code
getsentry-bot Sep 25, 2023
3418390
Merge branch 'main' into fix/access-UISession-main-thread
brustolin Sep 28, 2023
73db556
Update SentrySDKTests.swift
brustolin Sep 28, 2023
581b8df
Merge branch 'fix/access-UISession-main-thread' of github.com:getsent…
brustolin Sep 28, 2023
45b181b
Update SentrySDK.m
brustolin Sep 28, 2023
cd29f40
Format code
getsentry-bot Sep 28, 2023
fb5eebc
Update SentrySDKTests.swift
brustolin Sep 28, 2023
427f0a9
Merge branch 'fix/access-UISession-main-thread' of github.com:getsent…
brustolin Sep 28, 2023
e682529
Fix Reachability flackness
brustolin Sep 28, 2023
262c625
Format code
getsentry-bot Sep 28, 2023
d41ebfc
Update SentrySDK.m
brustolin Sep 28, 2023
60723f7
Merge branch 'fix/access-UISession-main-thread' of github.com:getsent…
brustolin Sep 28, 2023
3194c1d
Format code
getsentry-bot Sep 28, 2023
431978a
Merge branch 'main' into fix/access-UISession-main-thread
brustolin Oct 4, 2023
6692ff9
Update SentryReachability.m
brustolin Oct 4, 2023
aedd343
Format code
getsentry-bot Oct 4, 2023
cc4d0e3
Update SentrySDKTests.swift
brustolin Oct 5, 2023
f58236a
Update CHANGELOG.md
philipphofmann Oct 11, 2023
33e5dbe
ref: Replace deprecated function (#3327)
brustolin Oct 4, 2023
9bb1c59
ci: update unit test iOS/Xcode versions (#3283)
armcknight Oct 4, 2023
4815a2f
Fix: Remove unnecessaries build settings (#3325)
brustolin Oct 5, 2023
4ecd1bb
meta: update ruby version and gem/brew bundles (#3324)
armcknight Oct 5, 2023
86d4247
fix: remove relative component of import (#3332)
armcknight Oct 6, 2023
ff64ac7
rename replaceOptionIntegrations
philipphofmann Oct 11, 2023
b1292b1
Merge branch 'main' into fix/access-UISession-main-thread
philipphofmann Oct 11, 2023
d174abf
fix changelog
philipphofmann Oct 11, 2023
0c88c23
Make SDK init synchronous
philipphofmann Oct 11, 2023
e875917
change back to init async
philipphofmann Oct 11, 2023
0f5d42c
remove not needed file
philipphofmann Oct 11, 2023
d2a9730
simplify comment
philipphofmann Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Fixes

- Start SDK only in the main thread (#3291)
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved

## 8.12.0

### Fixes
Expand Down
4 changes: 4 additions & 0 deletions Sentry.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,7 @@
D86F419827C8FEFA00490520 /* SentryCoreDataTrackerExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = D86F419727C8FEFA00490520 /* SentryCoreDataTrackerExtension.swift */; };
D8751FA5274743710032F4DE /* SentryNSURLSessionTaskSearchTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D8751FA4274743710032F4DE /* SentryNSURLSessionTaskSearchTests.swift */; };
D875ED0B276CC84700422FAC /* SentryNSDataTrackerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D875ED0A276CC84700422FAC /* SentryNSDataTrackerTests.swift */; };
D87FA2892AB9EB06007DE933 /* MainThreadTestIntegration.swift in Sources */ = {isa = PBXBuildFile; fileRef = D87FA2882AB9EB06007DE933 /* MainThreadTestIntegration.swift */; };
D880E3A728573E87008A90DB /* SentryBaggageTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D880E3A628573E87008A90DB /* SentryBaggageTests.swift */; };
D884A20527C80F6300074664 /* SentryCoreDataTrackerTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = D884A20327C80F2700074664 /* SentryCoreDataTrackerTest.swift */; };
D885266427739D01001269FC /* SentryFileIOTrackingIntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D885266327739D01001269FC /* SentryFileIOTrackingIntegrationTests.swift */; };
Expand Down Expand Up @@ -1727,6 +1728,7 @@
D8751FA4274743710032F4DE /* SentryNSURLSessionTaskSearchTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryNSURLSessionTaskSearchTests.swift; sourceTree = "<group>"; };
D8757D142A209F7300BFEFCC /* SentrySampleDecision+Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = "SentrySampleDecision+Private.h"; path = "include/SentrySampleDecision+Private.h"; sourceTree = "<group>"; };
D875ED0A276CC84700422FAC /* SentryNSDataTrackerTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SentryNSDataTrackerTests.swift; sourceTree = "<group>"; };
D87FA2882AB9EB06007DE933 /* MainThreadTestIntegration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MainThreadTestIntegration.swift; sourceTree = "<group>"; };
D880E3A628573E87008A90DB /* SentryBaggageTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryBaggageTests.swift; sourceTree = "<group>"; };
D880E3B02860A5A0008A90DB /* SentryEvent+Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = "SentryEvent+Private.h"; path = "include/SentryEvent+Private.h"; sourceTree = "<group>"; };
D884A20327C80F2700074664 /* SentryCoreDataTrackerTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCoreDataTrackerTest.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -3180,6 +3182,7 @@
84281C4D2A579A0C00EE88F2 /* SentryProfilerMocksSwiftCompatible.mm */,
84AC61DA29F7654A009EEF61 /* TestDispatchSourceWrapper.swift */,
84A5D75A29D5170700388BFA /* TimeInterval+Sentry.swift */,
D87FA2882AB9EB06007DE933 /* MainThreadTestIntegration.swift */,
7B30B68126527C55006B2752 /* TestDisplayLinkWrapper.swift */,
8E25C97425F8511A00DC215B /* TestRandom.swift */,
7BE3C7762445E50A00A38442 /* TestCurrentDateProvider.swift */,
Expand Down Expand Up @@ -4482,6 +4485,7 @@
84B7FA4529B2926900AD93B1 /* TestDisplayLinkWrapper.swift in Sources */,
84AC61DB29F7654A009EEF61 /* TestDispatchSourceWrapper.swift in Sources */,
8431F01729B2851500D8DC56 /* TestSentrySystemWrapper.swift in Sources */,
D87FA2892AB9EB06007DE933 /* MainThreadTestIntegration.swift in Sources */,
84281C632A579D0700EE88F2 /* SentryProfilerMocks.mm in Sources */,
84B7FA4129B28CD200AD93B1 /* TestSentryDispatchQueueWrapper.swift in Sources */,
84B7FA3E29B28ADD00AD93B1 /* TestClient.swift in Sources */,
Expand Down
16 changes: 16 additions & 0 deletions SentryTestUtils/MainThreadTestIntegration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import Foundation
import Sentry

public class MainThreadTestIntegration: NSObject, SentryIntegrationProtocol {

public var installedInTheMainThread = false

public func install(with options: Options) -> Bool {
installedInTheMainThread = Thread.isMainThread
return true
}

public static func replaceOptionIntegrations(_ options: Options) {
philipphofmann marked this conversation as resolved.
Show resolved Hide resolved
options.integrations = [ NSStringFromClass(MainThreadTestIntegration.self) ]
}
}
38 changes: 24 additions & 14 deletions Sources/Sentry/SentrySDK.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#import "SentryCrash.h"
#import "SentryCrashWrapper.h"
#import "SentryDependencyContainer.h"
#import "SentryDispatchQueueWrapper.h"
#import "SentryFileManager.h"
#import "SentryHub+Private.h"
#import "SentryLog.h"
Expand Down Expand Up @@ -134,24 +135,33 @@ + (void)setStartInvocations:(NSUInteger)value

+ (void)startWithOptions:(SentryOptions *)options
{
startInvocations++;
void (^startSDK)(void) = ^{
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 (NSThread.isMainThread) {
startSDK();
} else {
[SentryDependencyContainer.sharedInstance.dispatchQueueWrapper
dispatchAsyncOnMainQueue:startSDK];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

+ (void)startWithConfigureOptions:(void (^)(SentryOptions *options))configureOptions
Expand Down
6 changes: 3 additions & 3 deletions Sources/SentryCrash/Recording/Tools/SentryCrashObjCApple.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,8 @@ typedef struct __CFRuntimeBase {
# define __CF_BIG_ENDIAN__ 0
#endif

#define CF_INFO_BITS (!!(__CF_BIG_ENDIAN__)*3)
#define CF_RC_BITS (!!(__CF_LITTLE_ENDIAN__)*3)
#define CF_INFO_BITS (!!(__CF_BIG_ENDIAN__) * 3)
armcknight marked this conversation as resolved.
Show resolved Hide resolved
#define CF_RC_BITS (!!(__CF_LITTLE_ENDIAN__) * 3)

/* Bit manipulation macros */
/* Bits are numbered from 31 on left to 0 on right */
Expand All @@ -299,7 +299,7 @@ typedef struct __CFRuntimeBase {
/* In the following, N1 and N2 specify an inclusive range N2..N1 with N1 >= N2
*/
#define __CFBitfieldMask(N1, N2) ((((UInt32)~0UL) << (31UL - (N1) + (N2))) >> (31UL - N1))
#define __CFBitfieldGetValue(V, N1, N2) (((V)&__CFBitfieldMask(N1, N2)) >> (N2))
#define __CFBitfieldGetValue(V, N1, N2) (((V) & __CFBitfieldMask(N1, N2)) >> (N2))

// ======================================================================
#pragma mark - CF-1153.18/CFString.c -
Expand Down
12 changes: 4 additions & 8 deletions Tests/SentryTests/SentryOptionsTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,10 +801,8 @@ - (void)testChanging_enableTracing_afterSetting_tracesSampleRate
- (void)testChanging_enableTracing_afterSetting_tracesSampler
{
SentryOptions *options = [[SentryOptions alloc] init];
options.tracesSampler = ^NSNumber *(SentrySamplingContext *__unused samplingContext)
{
return @0.1;
};
options.tracesSampler
= ^NSNumber *(SentrySamplingContext *__unused samplingContext) { return @0.1; };
options.enableTracing = NO;
XCTAssertNil(options.tracesSampleRate);
options.enableTracing = FALSE;
Expand Down Expand Up @@ -1161,10 +1159,8 @@ - (void)testDefaultInitialScope

- (void)testInitialScope
{
SentryScope * (^initialScope)(SentryScope *) = ^SentryScope *(SentryScope *scope)
{
return scope;
};
SentryScope * (^initialScope)(SentryScope *)
= ^SentryScope *(SentryScope *scope) { return scope; };
armcknight marked this conversation as resolved.
Show resolved Hide resolved
SentryOptions *options = [self getValidOptions:@{ @"initialScope" : initialScope }];
XCTAssertIdentical(initialScope, options.initialScope);
}
Expand Down
16 changes: 15 additions & 1 deletion Tests/SentryTests/SentrySDKTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,21 @@ class SentrySDKTests: XCTestCase {

XCTAssertEqual(flushTimeout, transport.flushInvocations.first)
}


func testStartInTheMainThread() {
let expect = expectation(description: "SDK Initialization")
DispatchQueue.global(qos: .background).async {
SentrySDK.start { options in
MainThreadTestIntegration.replaceOptionIntegrations(options)
}
expect.fulfill()
}
wait(for: [expect], timeout: 0.5)

let mainThreadIntegration = SentrySDK.currentHub().installedIntegrations().first as? MainThreadTestIntegration
XCTAssertEqual(mainThreadIntegration?.installedInTheMainThread, true, "SDK is not being initialized in the main thread")
}

#if SENTRY_HAS_UIKIT

func testSetAppStartMeasurementConcurrently() {
Expand Down