Skip to content

Commit 9de77c7

Browse files
authored
Merge pull request #4091 from sfdctaka/bugfix/notification-types-thread-safety
Fix thread-safety race on SFUserAccount.notificationTypes
2 parents 99a173b + ebeb583 commit 9de77c7

3 files changed

Lines changed: 176 additions & 2 deletions

File tree

libs/SalesforceSDKCore/SalesforceSDKCore.xcodeproj/project.pbxproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,7 @@
399399
CE7F662B1E556CA800DC3FBB /* SFNetwork.h in Headers */ = {isa = PBXBuildFile; fileRef = CE7F66291E556CA800DC3FBB /* SFNetwork.h */; settings = {ATTRIBUTES = (Public, ); }; };
400400
CE7F662C1E556CA800DC3FBB /* SFNetwork.m in Sources */ = {isa = PBXBuildFile; fileRef = CE7F662A1E556CA800DC3FBB /* SFNetwork.m */; };
401401
CE81A9C81E9C26F900F3D0AD /* SFUserAccountManagerNotificationsTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CE81A9C61E9C26EF00F3D0AD /* SFUserAccountManagerNotificationsTests.m */; };
402+
42996EA8CCEB33EF9E258033 /* SFUserAccountThreadSafetyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0DB0265069ECCAE8F59F5CD0 /* SFUserAccountThreadSafetyTests.swift */; };
402403
CE88BD521D17065C00AE3BF7 /* SFSDKAILTNPublisher.h in Headers */ = {isa = PBXBuildFile; fileRef = CE88BD4F1D17065B00AE3BF7 /* SFSDKAILTNPublisher.h */; settings = {ATTRIBUTES = (Public, ); }; };
403404
CE88BD531D17065C00AE3BF7 /* SFSDKAILTNPublisher.m in Sources */ = {isa = PBXBuildFile; fileRef = CE88BD501D17065C00AE3BF7 /* SFSDKAILTNPublisher.m */; };
404405
CE88BD541D17065C00AE3BF7 /* SFSDKAnalyticsPublisher.h in Headers */ = {isa = PBXBuildFile; fileRef = CE88BD511D17065C00AE3BF7 /* SFSDKAnalyticsPublisher.h */; settings = {ATTRIBUTES = (Public, ); }; };
@@ -933,6 +934,7 @@
933934
CE7F66291E556CA800DC3FBB /* SFNetwork.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SFNetwork.h; sourceTree = "<group>"; };
934935
CE7F662A1E556CA800DC3FBB /* SFNetwork.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SFNetwork.m; sourceTree = "<group>"; };
935936
CE81A9C61E9C26EF00F3D0AD /* SFUserAccountManagerNotificationsTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = SFUserAccountManagerNotificationsTests.m; path = SalesforceSDKCoreTests/SFUserAccountManagerNotificationsTests.m; sourceTree = SOURCE_ROOT; };
937+
0DB0265069ECCAE8F59F5CD0 /* SFUserAccountThreadSafetyTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; name = SFUserAccountThreadSafetyTests.swift; path = SalesforceSDKCoreTests/SFUserAccountThreadSafetyTests.swift; sourceTree = SOURCE_ROOT; };
936938
CE88BD4F1D17065B00AE3BF7 /* SFSDKAILTNPublisher.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SFSDKAILTNPublisher.h; path = Analytics/SFSDKAILTNPublisher.h; sourceTree = "<group>"; };
937939
CE88BD501D17065C00AE3BF7 /* SFSDKAILTNPublisher.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = SFSDKAILTNPublisher.m; path = Analytics/SFSDKAILTNPublisher.m; sourceTree = "<group>"; };
938940
CE88BD511D17065C00AE3BF7 /* SFSDKAnalyticsPublisher.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SFSDKAnalyticsPublisher.h; path = Analytics/SFSDKAnalyticsPublisher.h; sourceTree = "<group>"; };
@@ -1116,6 +1118,7 @@
11161118
CE81A9C61E9C26EF00F3D0AD /* SFUserAccountManagerNotificationsTests.m */,
11171119
B7E8A2AE1E77062E007C0D92 /* SFUserAccountManagerPersisterTests.m */,
11181120
4F06AF721C49A16A00F70798 /* SFUserAccountManagerTests.m */,
1121+
0DB0265069ECCAE8F59F5CD0 /* SFUserAccountThreadSafetyTests.swift */,
11191122
B7E8A2B01E770A57007C0D92 /* SFUserAccountPersisterEphemeral.h */,
11201123
B7E8A2B11E770A57007C0D92 /* SFUserAccountPersisterEphemeral.m */,
11211124
FDD7D7A0232039D000F5FB2D /* SFUserAccountPhotoTests.m */,
@@ -2218,6 +2221,7 @@
22182221
4F3ECD8C2EBBD182005020A6 /* SFOAuthInfoTests.m in Sources */,
22192222
4F7EB4161BFFC8D700768720 /* SDKCommonNSDataTests.m in Sources */,
22202223
CE81A9C81E9C26F900F3D0AD /* SFUserAccountManagerNotificationsTests.m in Sources */,
2224+
42996EA8CCEB33EF9E258033 /* SFUserAccountThreadSafetyTests.swift in Sources */,
22212225
23F200AC2E551C890091C5F5 /* ActionTypeTests.swift in Sources */,
22222226
4FAUTHFLOW001234567890ABC /* AuthFlowTypesViewTests.swift in Sources */,
22232227
23F200AE2E551C890091C5F5 /* BootconfigTests.swift in Sources */,

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/UserAccount/SFUserAccount.m

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,17 @@ - (void)setAccessScopes:(NSSet<NSString *> *)accessScopes {
145145

146146

147147
- (NSArray<NotificationType *> *)notificationTypes {
148-
return _notificationTypes;
148+
__block NSArray<NotificationType *> *types = nil;
149+
dispatch_sync(_syncQueue, ^{
150+
types = self->_notificationTypes;
151+
});
152+
return types;
149153
}
150154

151155
- (void)setNotificationTypes:(NSArray<NotificationType *> *)notificationTypes {
152-
_notificationTypes = [notificationTypes copy];
156+
dispatch_barrier_async(_syncQueue, ^{
157+
self->_notificationTypes = [notificationTypes copy];
158+
});
153159
}
154160

155161
- (NSString *)userPhotoDirectory {
Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
/*
2+
Copyright (c) 2017-present, salesforce.com, inc. All rights reserved.
3+
4+
Redistribution and use of this software in source and binary forms, with or without modification,
5+
are permitted provided that the following conditions are met:
6+
* Redistributions of source code must retain the above copyright notice, this list of conditions
7+
and the following disclaimer.
8+
* Redistributions in binary form must reproduce the above copyright notice, this list of
9+
conditions and the following disclaimer in the documentation and/or other materials provided
10+
with the distribution.
11+
* Neither the name of salesforce.com, inc. nor the names of its contributors may be used to
12+
endorse or promote products derived from this software without specific prior written
13+
permission of salesforce.com, inc.
14+
15+
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR
16+
IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
17+
FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
18+
CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
19+
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
20+
DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
21+
WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
22+
WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
23+
*/
24+
25+
import XCTest
26+
@testable import SalesforceSDKCore
27+
28+
class UserAccountThreadSafetyTests: XCTestCase {
29+
var account: UserAccount!
30+
var poolA: [NotificationType]!
31+
var poolB: [NotificationType]!
32+
33+
override func setUp() {
34+
super.setUp()
35+
36+
let credentials = OAuthCredentials(identifier: "thread-safety-test", clientId: "test-client-id", encrypted: true)!
37+
credentials.userId = "005R0000000ThreadSafety"
38+
credentials.organizationId = "00D000000ThreadSafety"
39+
credentials.identityUrl = URL(string: "https://login.salesforce.com/id/00D000000ThreadSafety/005R0000000ThreadSafety")
40+
account = UserAccount(credentials: credentials)
41+
42+
poolA = (0..<50).map { i in
43+
NotificationType(type: "type-A-\(i)", apiName: "TypeA\(i)", label: "Type A \(i)", actionGroups: nil)
44+
}
45+
46+
poolB = (0..<50).map { i in
47+
NotificationType(type: "type-B-\(i)", apiName: "TypeB\(i)", label: "Type B \(i)", actionGroups: nil)
48+
}
49+
}
50+
51+
override func tearDown() {
52+
account = nil
53+
poolA = nil
54+
poolB = nil
55+
super.tearDown()
56+
}
57+
58+
func test_notificationTypes_concurrent_reads_and_writes_do_not_crash() {
59+
let group = DispatchGroup()
60+
let queue = DispatchQueue.global(qos: .userInitiated)
61+
62+
for _ in 0..<8 {
63+
group.enter()
64+
queue.async {
65+
for i in 0..<10000 {
66+
self.account.notificationTypes = (i % 2 == 0) ? self.poolA : self.poolB
67+
}
68+
group.leave()
69+
}
70+
71+
group.enter()
72+
queue.async {
73+
for _ in 0..<10000 {
74+
let snapshot = self.account.notificationTypes
75+
_ = snapshot?.count
76+
}
77+
group.leave()
78+
}
79+
}
80+
81+
let result = group.wait(timeout: .now() + 30)
82+
XCTAssertEqual(.success, result, "Concurrent reads and writes timed out")
83+
}
84+
85+
func test_setNotificationTypes_takes_a_snapshot() {
86+
var mutableArray: [NotificationType] = []
87+
88+
let type1 = NotificationType(type: "type1", apiName: "Type1", label: "Type One", actionGroups: nil)
89+
let type2 = NotificationType(type: "type2", apiName: "Type2", label: "Type Two", actionGroups: nil)
90+
mutableArray.append(type1)
91+
mutableArray.append(type2)
92+
93+
account.notificationTypes = mutableArray
94+
95+
let snapshotAfterSet = account.notificationTypes
96+
XCTAssertEqual(snapshotAfterSet?.count, 2)
97+
98+
let type3 = NotificationType(type: "type3", apiName: "Type3", label: "Type Three", actionGroups: nil)
99+
mutableArray.append(type3)
100+
101+
let snapshotAfterMutate = account.notificationTypes
102+
XCTAssertEqual(snapshotAfterMutate?.count, 2, "Snapshot should remain unchanged after external mutation")
103+
}
104+
105+
func test_notificationTypes_returns_assigned_value_after_drain() {
106+
let arrayA = [NotificationType(type: "approval", apiName: "Approval", label: "Approval Request", actionGroups: nil)]
107+
108+
account.notificationTypes = arrayA
109+
let readA = account.notificationTypes
110+
XCTAssertEqual(readA?.count, arrayA.count)
111+
XCTAssertEqual(readA?.first?.type, arrayA.first?.type)
112+
113+
let arrayB = [
114+
NotificationType(type: "task", apiName: "Task", label: "Task Assignment", actionGroups: nil),
115+
NotificationType(type: "case", apiName: "Case", label: "Case Update", actionGroups: nil)
116+
]
117+
118+
account.notificationTypes = arrayB
119+
let readB = account.notificationTypes
120+
XCTAssertEqual(readB?.count, arrayB.count)
121+
XCTAssertEqual(readB?.first?.type, arrayB.first?.type)
122+
}
123+
124+
func test_encodeWithCoder_under_contention_does_not_throw() {
125+
let group = DispatchGroup()
126+
let queue = DispatchQueue.global(qos: .userInitiated)
127+
128+
group.enter()
129+
queue.async {
130+
for i in 0..<5000 {
131+
self.account.notificationTypes = (i % 2 == 0) ? self.poolA : self.poolB
132+
}
133+
group.leave()
134+
}
135+
136+
var lastEncodedData: Data?
137+
138+
for _ in 0..<500 {
139+
do {
140+
let data = try NSKeyedArchiver.archivedData(withRootObject: account!, requiringSecureCoding: true)
141+
XCTAssertFalse(data.isEmpty, "Encode should succeed under contention")
142+
lastEncodedData = data
143+
} catch {
144+
XCTFail("Encode should not throw exception: \(error)")
145+
}
146+
}
147+
148+
let result = group.wait(timeout: .now() + 30)
149+
XCTAssertEqual(.success, result, "Background write thread timed out")
150+
151+
if let data = lastEncodedData {
152+
do {
153+
let decoded = try NSKeyedUnarchiver.unarchivedObject(ofClass: UserAccount.self, from: data)
154+
XCTAssertNotNil(decoded, "Should decode last encoded account")
155+
156+
if let types = decoded?.notificationTypes {
157+
XCTAssertFalse(types.isEmpty, "Decoded notificationTypes should not be empty under contention")
158+
}
159+
} catch {
160+
XCTFail("Decode should not error: \(error)")
161+
}
162+
}
163+
}
164+
}

0 commit comments

Comments
 (0)