Merge pull request #4174 from wmathurin/fix-safe-collection-write-starvation

Fix safe collection write starvation under concurrent REST requests

Author: wmathurin

libs/SalesforceSDKCommon/SalesforceSDKCommon/Classes/Util/SFSDKSafeMutableDictionary.m

@@ -38,6 +38,7 @@ - (instancetype)init {
     self = [super init];
     if (self) {
         self.backingDictionary = [NSMutableDictionary new];
+        // A concurrent queue keeps independent readers parallel; barrier writes serialize with them.
         self.queue = dispatch_queue_create([NSString stringWithFormat:@"com.salesforce.mobilesdk.readWriteQueue%u", arc4random_uniform(UINT32_MAX)].UTF8String, DISPATCH_QUEUE_CONCURRENT);
     }
     return self;
@@ -94,7 +95,7 @@ - (void)setObject:(id)object forKey:(id<NSCopying>)aKey {
         [SFLogger w:[self class] format:@"Attempted to set object with nil key in safe dictionary"];
         return;
     }
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         self.backingDictionary[aKey] = object;
     });
 }
@@ -108,35 +109,33 @@ - (void)removeObject:(id<NSCopying>)aKey {
         [SFLogger w:[self class] format:@"Attempted to remove nil key from safe dictionary"];
         return;
     }
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingDictionary removeObjectForKey:aKey];
     });
 }
 
 - (void)removeAllObjects {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingDictionary removeAllObjects];
     });
 }
 
 - (void)removeObjects:(NSArray<id<NSCopying>> *)keys {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingDictionary removeObjectsForKeys:keys];
     });
 }
 
 - (void)addEntries:(NSDictionary *)otherDictionary {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingDictionary addEntriesFromDictionary:otherDictionary];
     });
 }
 
 - (void)setDictionary:(NSDictionary *)dictionary {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingDictionary setDictionary:dictionary];
     });
 }
 
 @end
-
-

libs/SalesforceSDKCommon/SalesforceSDKCommon/Classes/Util/SFSDKSafeMutableSet.h

@@ -81,7 +81,9 @@ NS_SWIFT_NAME(SafeMutableSet)
 - (void)filterUsingPredicate:(NSPredicate *)predicate;
 
 /**
- * Enumerate objects in the set safely, using a block.
+ * Enumerates a stable snapshot of the set using a block. The block executes outside the
+ * collection's synchronization queue, so it may safely mutate the set. Mutations made while
+ * enumeration is in progress do not change the snapshot and may interleave with block calls.
  */
 - (void)enumerateObjectsUsingBlock:(void (^)(id obj, BOOL *stop))block;
 

libs/SalesforceSDKCommon/SalesforceSDKCommon/Classes/Util/SFSDKSafeMutableSet.m

@@ -101,66 +101,67 @@ - (BOOL)isEqualToSet:(SFSDKSafeMutableSet *)that {
 }
 
 - (void)addObject:(id)obj {
-    dispatch_barrier_async(self.queue, ^{
+    // Admission callers can immediately perform a synchronous membership read.
+    // Publish the mutation before returning so a burst of callers cannot fill
+    // their executor with readers waiting behind queued barrier writes.
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet addObject:obj];
     });
 }
 
 - (void)addObjectsFromArray:(NSArray *)array {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
          [self.backingSet addObjectsFromArray:array];
     });
 }
 
 - (void)removeAllObjects {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet removeAllObjects];
     });
 }
 
 - (void)removeObject:(id)object {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet removeObject:object];
     });
 }
 
 - (void)unionSet:(NSSet *)set {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet unionSet:set];
     });
 }
 
 - (void)minusSet:(NSSet *)set {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet minusSet:set];
     });
 }
 
 - (void)intersectSet:(NSSet *)set {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet intersectSet:set];
     });
 }
 
 - (void)setSet:(NSSet *)set {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet setSet:set];
     });
 }
 
 - (void)filterUsingPredicate:(NSPredicate *)predicate {
-    dispatch_barrier_async(self.queue, ^{
+    dispatch_barrier_sync(self.queue, ^{
         [self.backingSet filterUsingPredicate:predicate];
     });
 }
 
 - (void)enumerateObjectsUsingBlock:(void (^)(id obj, BOOL *stop))block {
-    __block NSArray *array = [self allObjects];
-    dispatch_barrier_sync(self.queue, ^{
-        [array enumerateObjectsUsingBlock:^(id  _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
-            block(obj, stop);
-        }];
-    });
+    NSArray *array = [self allObjects];
+    [array enumerateObjectsUsingBlock:^(id  _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
+        block(obj, stop);
+    }];
 }
 
 #pragma mark - Class Level
@@ -177,6 +178,7 @@ + (id)setWithCapacity:(NSUInteger)numItems {
 
 #pragma private methods
 - (void)initQueue {
+    // A concurrent queue keeps independent readers parallel; barrier writes serialize with them.
      self.queue = dispatch_queue_create([NSString stringWithFormat:@"com.salesforce.mobilesdk.readWriteSetQ%u", arc4random_uniform(UINT32_MAX)].UTF8String, DISPATCH_QUEUE_CONCURRENT);
 }
 

libs/SalesforceSDKCommon/SalesforceSDKCommonTests/SFSDKSafeMutableDictionaryTests.m

@@ -25,6 +25,31 @@
 @import XCTest;
 #import "SFSDKSafeMutableDictionary.h"
 
+// Blocking -hash pauses inside NSMutableDictionary's mutation so publication timing is deterministic.
+@interface SFSDKBlockingDictionaryKey : NSObject <NSCopying>
+@property (nonatomic, strong) dispatch_semaphore_t hashStarted;
+@property (nonatomic, strong) dispatch_semaphore_t allowHashToReturn;
+@property (atomic, assign) BOOL shouldBlockHash;
+@end
+
+@implementation SFSDKBlockingDictionaryKey
+
+- (id)copyWithZone:(NSZone *)zone {
+    return self;
+}
+
+- (NSUInteger)hash {
+    if (self.shouldBlockHash) {
+        self.shouldBlockHash = NO;
+        dispatch_semaphore_signal(self.hashStarted);
+        dispatch_semaphore_wait(self.allowHashToReturn,
+                                dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC)));
+    }
+    return 1;
+}
+
+@end
+
 @interface SFSDKSafeMutableDictionaryTests : XCTestCase
 
 @property (strong, nonatomic) SFSDKSafeMutableDictionary *testDictionary;
@@ -64,6 +89,32 @@ - (void)testConcurrentReadWrites {
     }];
 }
 
+- (void)testSetObjectPublishesMutationBeforeReturning {
+    SFSDKSafeMutableDictionary *dictionary = [[SFSDKSafeMutableDictionary alloc] init];
+    SFSDKBlockingDictionaryKey *key = [SFSDKBlockingDictionaryKey new];
+    key.hashStarted = dispatch_semaphore_create(0);
+    key.allowHashToReturn = dispatch_semaphore_create(0);
+    key.shouldBlockHash = YES;
+    dispatch_semaphore_t setReturned = dispatch_semaphore_create(0);
+
+    dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{
+        [dictionary setObject:@"value" forKey:key];
+        dispatch_semaphore_signal(setReturned);
+    });
+
+    XCTAssertEqual(dispatch_semaphore_wait(key.hashStarted,
+                                           dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC))), 0,
+                   @"dictionary mutation should begin");
+    XCTAssertNotEqual(dispatch_semaphore_wait(setReturned, DISPATCH_TIME_NOW), 0,
+                      @"setObject:forKey: must not return before its mutation is published");
+
+    dispatch_semaphore_signal(key.allowHashToReturn);
+    XCTAssertEqual(dispatch_semaphore_wait(setReturned,
+                                           dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC))), 0,
+                   @"setObject:forKey: should return after publishing the mutation");
+    XCTAssertEqualObjects([dictionary objectForKey:key], @"value");
+}
+
 #pragma Mark - Helper Methods
 
 - (NSArray *)generateTestKeys {

libs/SalesforceSDKCommon/SalesforceSDKCommonTests/SFSDKSafeMutableSetTests.m

@@ -25,6 +25,27 @@
 @import XCTest;
 #import "SFSDKSafeMutableSet.h"
 
+// Blocking -hash pauses inside NSMutableSet's mutation so publication timing is deterministic.
+@interface SFSDKBlockingHashObject : NSObject
+@property (nonatomic, strong) dispatch_semaphore_t hashStarted;
+@property (nonatomic, strong) dispatch_semaphore_t allowHashToReturn;
+@property (atomic, assign) BOOL shouldBlockHash;
+@end
+
+@implementation SFSDKBlockingHashObject
+
+- (NSUInteger)hash {
+    if (self.shouldBlockHash) {
+        self.shouldBlockHash = NO;
+        dispatch_semaphore_signal(self.hashStarted);
+        dispatch_semaphore_wait(self.allowHashToReturn,
+                                dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC)));
+    }
+    return 1;
+}
+
+@end
+
 @interface SFSDKSafeMutableSetTests : XCTestCase
 @end
 
@@ -76,6 +97,58 @@ - (void)testConcurrentWrites {
     }];
 }
 
+- (void)testAddObjectPublishesMutationBeforeReturning {
+    SFSDKSafeMutableSet *set = [SFSDKSafeMutableSet set];
+    SFSDKBlockingHashObject *object = [SFSDKBlockingHashObject new];
+    object.hashStarted = dispatch_semaphore_create(0);
+    object.allowHashToReturn = dispatch_semaphore_create(0);
+    object.shouldBlockHash = YES;
+    dispatch_semaphore_t addReturned = dispatch_semaphore_create(0);
+
+    dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{
+        [set addObject:object];
+        dispatch_semaphore_signal(addReturned);
+    });
+
+    XCTAssertEqual(dispatch_semaphore_wait(object.hashStarted,
+                                           dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC))), 0,
+                   @"set mutation should begin");
+    XCTAssertNotEqual(dispatch_semaphore_wait(addReturned, DISPATCH_TIME_NOW), 0,
+                      @"addObject: must not return before its mutation is published");
+
+    dispatch_semaphore_signal(object.allowHashToReturn);
+    XCTAssertEqual(dispatch_semaphore_wait(addReturned,
+                                           dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2 * NSEC_PER_SEC))), 0,
+                   @"addObject: should return after publishing the mutation");
+    XCTAssertTrue([set containsObject:object]);
+}
+
+- (void)testEnumerationCallbackCanMutateSet {
+    SFSDKSafeMutableSet *set = [SFSDKSafeMutableSet set];
+    [set addObjectsFromArray:@[@"one", @"two"]];
+    NSMutableSet *enumeratedObjects = [NSMutableSet set];
+    XCTestExpectation *enumerationCompleted = [self expectationWithDescription:@"enumerationCompleted"];
+
+    dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{
+        [set enumerateObjectsUsingBlock:^(id obj, BOOL *stop) {
+            [enumeratedObjects addObject:obj];
+            [set addObject:@"addedFromCallback"];
+        }];
+        [enumerationCompleted fulfill];
+    });
+
+    XCTWaiterResult result = [XCTWaiter waitForExpectations:@[enumerationCompleted] timeout:2.0];
+    XCTAssertEqual(result, XCTWaiterResultCompleted,
+                   @"enumeration callbacks must be able to mutate the set without deadlocking");
+    if (result != XCTWaiterResultCompleted) {
+        return;
+    }
+
+    NSSet *expectedObjects = [NSSet setWithObjects:@"one", @"two", nil];
+    XCTAssertEqualObjects(enumeratedObjects, expectedObjects);
+    XCTAssertTrue([set containsObject:@"addedFromCallback"]);
+}
+
 - (void)testConcurrentReadWrites {
     NSArray *inputs = @[@"Test1", @"Test2", @"Test3", @"Test4", @"Test5"];