Replace @synchronized with NSRecursiveLock

Created by: rgurzhiy

This pull request replaces @synchronized with NSRecursiveLock as in #249 we've discovered that @synchronized holds unretained allocations. The replacement is pretty much straightforward. I've just changed

@synchronized (self.lock) {
...
}

with

[self.lock lock];
...
[self.lock unlock];

and removed all returns in between.

There are still 2 places I'm not sure about: BFTask

- (void)waitUntilFinished {
    if ([NSThread isMainThread]) {
        [self warnOperationOnMainThread];
    }

    [self.lock lock];
    if (self.completed) {
        [self.lock unlock];
        return;
    }
    [self.condition lock];
    [self.lock unlock];
...

BFCancellationTokenRegistration

- (void)dispose {
    [self.lock lock];
    if (self.disposed) {
        [self.lock unlock];
        return;
    }
    self.disposed = YES;
    [self.lock unlock];
...

Bolts/Common/BFCancellationToken.m

@@ -16,7 +16,7 @@ NS_ASSUME_NONNULL_BEGIN
 @interface BFCancellationToken ()
 
 @property (nullable, nonatomic, strong) NSMutableArray *registrations;
-@property (nonatomic, strong) NSObject *lock;
+@property (nonatomic, strong) NSRecursiveLock *lock;
 @property (nonatomic) BOOL disposed;
 
 @end
@@ -40,7 +40,7 @@ NS_ASSUME_NONNULL_BEGIN
     if (!self) return self;
 
     _registrations = [NSMutableArray array];
-    _lock = [NSObject new];
+    _lock = [NSRecursiveLock new];
 
     return self;
 }
@@ -48,23 +48,25 @@ NS_ASSUME_NONNULL_BEGIN
 #pragma mark - Custom Setters/Getters
 
 - (BOOL)isCancellationRequested {
-    @synchronized(self.lock) {
-        [self throwIfDisposed];
-        return _cancellationRequested;
-    }
+    [self.lock lock];
+    [self throwIfDisposed];
+    BOOL returnValue = _cancellationRequested;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (void)cancel {
     NSArray *registrations;
-    @synchronized(self.lock) {
-        [self throwIfDisposed];
-        if (_cancellationRequested) {
-            return;
-        }
+    
+    [self.lock lock];
+    [self throwIfDisposed];
+    if (!_cancellationRequested) {
         [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(cancelPrivate) object:nil];
         _cancellationRequested = YES;
         registrations = [self.registrations copy];
     }
+    [self.lock unlock];
 
     [self notifyCancellation:registrations];
 }
@@ -76,19 +78,18 @@ NS_ASSUME_NONNULL_BEGIN
 }
 
 - (BFCancellationTokenRegistration *)registerCancellationObserverWithBlock:(BFCancellationBlock)block {
-    @synchronized(self.lock) {
-        BFCancellationTokenRegistration *registration = [BFCancellationTokenRegistration registrationWithToken:self delegate:[block copy]];
-        [self.registrations addObject:registration];
-
-        return registration;
-    }
+    [self.lock lock];
+    BFCancellationTokenRegistration *registration = [BFCancellationTokenRegistration registrationWithToken:self delegate:[block copy]];
+    [self.registrations addObject:registration];
+    [self.lock unlock];
+    return registration;
 }
 
 - (void)unregisterRegistration:(BFCancellationTokenRegistration *)registration {
-    @synchronized(self.lock) {
-        [self throwIfDisposed];
-        [self.registrations removeObject:registration];
-    }
+    [self.lock lock];
+    [self throwIfDisposed];
+    [self.registrations removeObject:registration];
+    [self.lock unlock];
 }
 
 // Delay on a non-public method to prevent interference with a user calling performSelector or
@@ -108,29 +109,26 @@ NS_ASSUME_NONNULL_BEGIN
         return;
     }
 
-    @synchronized(self.lock) {
-        [self throwIfDisposed];
-        [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(cancelPrivate) object:nil];
-        if (self.cancellationRequested) {
-            return;
-        }
-
+    [self.lock lock];
+    [self throwIfDisposed];
+    [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(cancelPrivate) object:nil];
+    if (!self.cancellationRequested) {
         if (millis != -1) {
             double delay = (double)millis / 1000;
             [self performSelector:@selector(cancelPrivate) withObject:nil afterDelay:delay];
         }
     }
+    [self.lock unlock];
 }
 
 - (void)dispose {
-    @synchronized(self.lock) {
-        if (self.disposed) {
-            return;
-        }
+    [self.lock lock];
+    if (!self.disposed) {
         [self.registrations makeObjectsPerformSelector:@selector(dispose)];
         self.registrations = nil;
         self.disposed = YES;
     }
+    [self.lock unlock];
 }
 
 - (void)throwIfDisposed {

Bolts/Common/BFCancellationTokenRegistration.m

@@ -18,7 +18,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 @property (nonatomic, weak) BFCancellationToken *token;
 @property (nullable, nonatomic, strong) BFCancellationBlock cancellationObserverBlock;
-@property (nonatomic, strong) NSObject *lock;
+@property (nonatomic, strong) NSRecursiveLock *lock;
 @property (nonatomic) BOOL disposed;
 
 @end
@@ -42,19 +42,20 @@ NS_ASSUME_NONNULL_BEGIN
     self = [super init];
     if (!self) return self;
 
-    _lock = [NSObject new];
+    _lock = [NSRecursiveLock new];
     
     return self;
 }
 
 - (void)dispose {
-    @synchronized(self.lock) {
-        if (self.disposed) {
-            return;
-        }
-        self.disposed = YES;
+    [self.lock lock];
+    if (self.disposed) {
+        [self.lock unlock];
+        return;
     }
-
+    self.disposed = YES;
+    [self.lock unlock];
+    
     BFCancellationToken *token = self.token;
     if (token != nil) {
         [token unregisterRegistration:self];
@@ -64,10 +65,10 @@ NS_ASSUME_NONNULL_BEGIN
 }
 
 - (void)notifyDelegate {
-    @synchronized(self.lock) {
-        [self throwIfDisposed];
-        self.cancellationObserverBlock();
-    }
+    [self.lock lock];
+    [self throwIfDisposed];
+    self.cancellationObserverBlock();
+    [self.lock unlock];
 }
 
 - (void)throwIfDisposed {

Bolts/Common/BFTask.m

@@ -39,7 +39,7 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
 @property (nonatomic, assign, readwrite, getter=isFaulted) BOOL faulted;
 @property (nonatomic, assign, readwrite, getter=isCompleted) BOOL completed;
 
-@property (nonatomic, strong) NSObject *lock;
+@property (nonatomic, strong) NSRecursiveLock *lock;
 @property (nonatomic, strong) NSCondition *condition;
 @property (nonatomic, strong) NSMutableArray *callbacks;
 
@@ -53,7 +53,7 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
     self = [super init];
     if (!self) return self;
 
-    _lock = [[NSObject alloc] init];
+    _lock = [[NSRecursiveLock alloc] init];
     _condition = [[NSCondition alloc] init];
     _callbacks = [NSMutableArray array];
 
@@ -121,7 +121,7 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
     }
 
     __block int32_t cancelled = 0;
-    NSObject *lock = [[NSObject alloc] init];
+    NSRecursiveLock *lock = [[NSRecursiveLock alloc] init];
     NSMutableArray *errors = [NSMutableArray array];
     NSMutableArray *exceptions = [NSMutableArray array];
 
@@ -131,14 +131,14 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
             if (task.exception) {
-                @synchronized (lock) {
-                    [exceptions addObject:task.exception];
+                [lock lock];
+                [exceptions addObject:task.exception];
+                [lock unlock];
 #pragma clang diagnostic pop
-                }
             } else if (task.error) {
-                @synchronized (lock) {
-                    [errors addObject:task.error];
-                }
+                [lock lock];
+                [errors addObject:task.error];
+                [lock unlock];
             } else if (task.cancelled) {
                 OSAtomicIncrement32Barrier(&cancelled);
             }
@@ -193,7 +193,7 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
     __block int completed = 0;
     __block int32_t cancelled = 0;
     
-    NSObject *lock = [NSObject new];
+    NSRecursiveLock *lock = [NSRecursiveLock new];
     NSMutableArray<NSError *> *errors = [NSMutableArray new];
     NSMutableArray<NSException *> *exceptions = [NSMutableArray new];
     
@@ -203,14 +203,14 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
             if (task.exception != nil) {
-                @synchronized(lock) {
-                    [exceptions addObject:task.exception];
+                [lock lock];
+                [exceptions addObject:task.exception];
+                [lock unlock];
 #pragma clang diagnostic pop
-                }
             } else if (task.error != nil) {
-                @synchronized(lock) {
-                    [errors addObject:task.error];
-                }
+                [lock lock];
+                [errors addObject:task.error];
+                [lock unlock];
             } else if (task.cancelled) {
                 OSAtomicIncrement32Barrier(&cancelled);
             } else {
@@ -289,101 +289,126 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
 #pragma mark - Custom Setters/Getters
 
 - (nullable id)result {
-    @synchronized(self.lock) {
-        return _result;
-    }
+    [self.lock lock];
+    id returnValue = _result;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (BOOL)trySetResult:(nullable id)result {
-    @synchronized(self.lock) {
-        if (self.completed) {
-            return NO;
-        }
+    [self.lock lock];
+    
+    BOOL returnValue = NO;
+    if (!self.completed) {
         self.completed = YES;
         _result = result;
         [self runContinuations];
-        return YES;
+        returnValue = YES;
     }
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (nullable NSError *)error {
-    @synchronized(self.lock) {
-        return _error;
-    }
+    [self.lock lock];
+    NSError *returnValue = _error;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (BOOL)trySetError:(NSError *)error {
-    @synchronized(self.lock) {
-        if (self.completed) {
-            return NO;
-        }
+    [self.lock lock];
+    
+    BOOL returnValue = NO;
+    if (!self.completed) {
         self.completed = YES;
         self.faulted = YES;
         _error = error;
         [self runContinuations];
-        return YES;
+        returnValue = YES;
     }
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (nullable NSException *)exception {
-    @synchronized(self.lock) {
-        return _exception;
-    }
+    [self.lock lock];
+    NSException *returnValue = _exception;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (BOOL)trySetException:(NSException *)exception {
-    @synchronized(self.lock) {
-        if (self.completed) {
-            return NO;
-        }
+    [self.lock lock];
+    BOOL returnValue = NO;
+    
+    if (!self.completed) {
         self.completed = YES;
         self.faulted = YES;
         _exception = exception;
         [self runContinuations];
-        return YES;
+        returnValue = YES;
     }
+    
+    [self.lock unlock];
+    return returnValue;
+
 }
 
 - (BOOL)isCancelled {
-    @synchronized(self.lock) {
-        return _cancelled;
-    }
+    [self.lock lock];
+    BOOL returnValue = _cancelled;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (BOOL)isFaulted {
-    @synchronized(self.lock) {
-        return _faulted;
-    }
+    [self.lock lock];
+    BOOL returnValue = _faulted;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (BOOL)trySetCancelled {
-    @synchronized(self.lock) {
-        if (self.completed) {
-            return NO;
-        }
+    [self.lock lock];
+    
+    BOOL returnValue = NO;
+    if (!self.completed) {
         self.completed = YES;
         self.cancelled = YES;
         [self runContinuations];
-        return YES;
+        returnValue = YES;
     }
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (BOOL)isCompleted {
-    @synchronized(self.lock) {
-        return _completed;
-    }
+    [self.lock lock];
+    BOOL returnValue = _completed;
+    
+    [self.lock unlock];
+    return returnValue;
 }
 
 - (void)runContinuations {
-    @synchronized(self.lock) {
-        [self.condition lock];
-        [self.condition broadcast];
-        [self.condition unlock];
-        for (void (^callback)() in self.callbacks) {
-            callback();
-        }
-        [self.callbacks removeAllObjects];
+    [self.lock lock];
+    [self.condition lock];
+    [self.condition broadcast];
+    [self.condition unlock];
+    for (void (^callback)() in self.callbacks) {
+        callback();
     }
+    [self.callbacks removeAllObjects];
+    [self.lock unlock];
 }
 
 #pragma mark - Chaining methods
@@ -454,14 +479,15 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
     };
 
     BOOL completed;
-    @synchronized(self.lock) {
-        completed = self.completed;
-        if (!completed) {
-            [self.callbacks addObject:[^{
-                [executor execute:executionBlock];
-            } copy]];
-        }
+    [self.lock lock];
+    completed = self.completed;
+    if (!completed) {
+        [self.callbacks addObject:[^{
+            [executor execute:executionBlock];
+        } copy]];
     }
+    [self.lock unlock];
+    
     if (completed) {
         [executor execute:executionBlock];
     }
@@ -516,13 +542,15 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
     if ([NSThread isMainThread]) {
         [self warnOperationOnMainThread];
     }
-
-    @synchronized(self.lock) {
-        if (self.completed) {
-            return;
-        }
-        [self.condition lock];
+    
+    [self.lock lock];
+    if (self.completed) {
+        [self.lock unlock];
+        return;
     }
+    [self.condition lock];
+    [self.lock unlock];
+    
     while (!self.completed) {
         [self.condition wait];
     }
@@ -538,12 +566,12 @@ NSString *const BFTaskMultipleExceptionsUserInfoKey = @"exceptions";
     BOOL faulted;
     NSString *resultDescription = nil;
 
-    @synchronized(self.lock) {
-        completed = self.completed;
-        cancelled = self.cancelled;
-        faulted = self.faulted;
-        resultDescription = completed ? [NSString stringWithFormat:@" result = %@", self.result] : @"";
-    }
+    [self.lock lock];
+    completed = self.completed;
+    cancelled = self.cancelled;
+    faulted = self.faulted;
+    resultDescription = completed ? [NSString stringWithFormat:@" result = %@", self.result] : @"";
+    [self.lock unlock];
 
     // Description string includes status information and, if available, the
     // result since in some ways this is what a promise actually "is".