Skip to content

Commit

Permalink
fix: Potential deadlock when starting the SDK (#3970)
Browse files Browse the repository at this point in the history
Fix a deadlock when two threads access SentrySDK.options and
SentrySDK.currentHub, which used the same object in synchronized.
This problem is fixed now by using two independent locks for
SentrySDK.options and SentrySDK.currentHub.

Fixes GH-3956, Fixes GH-3899
  • Loading branch information
philipphofmann committed May 13, 2024
1 parent e072ad1 commit 3b4110a
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

- Fix data race when calling reportFullyDisplayed from a background thread (#3926)
- Ensure flushing envelopes directly after capturing them (#3915)
- Potential deadlock when starting the SDK (#3970)

### Improvements

Expand Down
12 changes: 8 additions & 4 deletions Sources/Sentry/SentrySDK.m
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@
@implementation SentrySDK

static SentryHub *_Nullable currentHub;
static NSObject *currentHubLock;
static BOOL crashedLastRunCalled;
static SentryAppStartMeasurement *sentrySDKappStartMeasurement;
static NSObject *sentrySDKappStartMeasurementLock;
static BOOL _detectedStartUpCrash;
static SentryOptions *_Nullable startOption;
static NSObject *startOptionsLock;

/**
* @brief We need to keep track of the number of times @c +[startWith...] is called, because our OOM
Expand All @@ -65,14 +67,16 @@ + (void)initialize
{
if (self == [SentrySDK class]) {
sentrySDKappStartMeasurementLock = [[NSObject alloc] init];
currentHubLock = [[NSObject alloc] init];
startOptionsLock = [[NSObject alloc] init];
startInvocations = 0;
_detectedStartUpCrash = NO;
}
}

+ (SentryHub *)currentHub
{
@synchronized(self) {
@synchronized(currentHubLock) {
if (nil == currentHub) {
currentHub = [[SentryHub alloc] initWithClient:nil andScope:nil];
}
Expand All @@ -82,22 +86,22 @@ + (SentryHub *)currentHub

+ (nullable SentryOptions *)options
{
@synchronized(self) {
@synchronized(startOptionsLock) {
return startOption;
}
}

/** Internal, only needed for testing. */
+ (void)setCurrentHub:(nullable SentryHub *)hub
{
@synchronized(self) {
@synchronized(currentHubLock) {
currentHub = hub;
}
}
/** Internal, only needed for testing. */
+ (void)setStartOptions:(nullable SentryOptions *)options
{
@synchronized(self) {
@synchronized(startOptionsLock) {
startOption = options;
}
}
Expand Down
35 changes: 35 additions & 0 deletions Tests/SentryTests/SentrySDKTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,41 @@ class SentrySDKTests: XCTestCase {
}
}

/// Tests in this class aren't part of SentrySDKTests because we need would need to undo a bunch of operations
/// that are done in the setup.
class SentrySDKWithSetupTests: XCTestCase {

func testAccessingHubAndOptions_NoDeadlock() {
SentryLog.withOutLogs {

let concurrentQueue = DispatchQueue(label: "concurrent", attributes: .concurrent)

let expectation = expectation(description: "no deadlock")
expectation.expectedFulfillmentCount = 20

SentrySDK.setStart(Options())

for _ in 0..<10 {
concurrentQueue.async {
SentrySDK.currentHub().capture(message: "mess")
SentrySDK.setCurrentHub(nil)

expectation.fulfill()
}

concurrentQueue.async {
let hub = SentryHub(client: nil, andScope: nil)
expect(hub) != nil

expectation.fulfill()
}
}

wait(for: [expectation], timeout: 5.0)
}
}
}

public class MainThreadTestIntegration: NSObject, SentryIntegrationProtocol {

static var expectation: XCTestExpectation?
Expand Down

0 comments on commit 3b4110a

Please sign in to comment.