Merge pull request ReactiveCocoa#1456 from talko/serial

Fix lack of thread safety in -[RACSerialDisposable disposable]

Author: kastiglione

ReactiveCocoaFramework/ReactiveCocoa/RACSerialDisposable.m

@@ -10,17 +10,19 @@
 #import <libkern/OSAtomic.h>
 
 @interface RACSerialDisposable () {
-	// A reference to the receiver's `disposable`. This variable must only be
-	// modified atomically.
-	//
-	// If this is `self`, no `disposable` has been set, but the receiver has not
-	// been disposed of yet. `self` is never stored retained.
-	//
-	// If this is `nil`, the receiver has been disposed.
+	// The receiver's `disposable`. This variable must only be referenced while
+	// _spinLock is held.
+	RACDisposable * _disposable;
+
+	// YES if the receiver has been disposed. This variable must only be modified
+	// while _spinLock is held.
+	BOOL _disposed;
+
+	// A spinlock to protect access to _disposable and _disposed.
 	//
-	// Otherwise, this is a retained reference to the inner disposable and the
-	// receiver has not been disposed of yet.
-	void * volatile _disposablePtr;
+	// It must be used when _disposable is mutated or retained and when _disposed
+	// is mutated.
+	OSSpinLock _spinLock;
 }
 
 @end
@@ -30,12 +32,17 @@ @implementation RACSerialDisposable
 #pragma mark Properties
 
 - (BOOL)isDisposed {
-	return _disposablePtr == nil;
+	return _disposed;
 }
 
 - (RACDisposable *)disposable {
-	RACDisposable *disposable = (__bridge id)_disposablePtr;
-	return (disposable == self ? nil : disposable);
+	RACDisposable *result;
+
+	OSSpinLockLock(&_spinLock);
+	result = _disposable;
+	OSSpinLockUnlock(&_spinLock);
+
+	return result;
 }
 
 - (void)setDisposable:(RACDisposable *)disposable {
@@ -50,16 +57,6 @@ + (instancetype)serialDisposableWithDisposable:(RACDisposable *)disposable {
 	return serialDisposable;
 }
 
-- (id)init {
-	self = [super init];
-	if (self == nil) return nil;
-
-	_disposablePtr = (__bridge void *)self;
-	OSMemoryBarrier();
-
-	return self;
-}
-
 - (id)initWithBlock:(void (^)(void))block {
 	self = [self init];
 	if (self == nil) return nil;
@@ -69,65 +66,42 @@ - (id)initWithBlock:(void (^)(void))block {
 	return self;
 }
 
-- (void)dealloc {
-	self.disposable = nil;
-}
-
 #pragma mark Inner Disposable
 
 - (RACDisposable *)swapInDisposable:(RACDisposable *)newDisposable {
-	void * const selfPtr = (__bridge void *)self;
-
-	// Only retain the new disposable if it's not `self`.
-	// Take ownership before attempting the swap so that a subsequent swap
-	// receives an owned reference.
-	void *newDisposablePtr = selfPtr;
-	if (newDisposable != nil) {
-		newDisposablePtr = (void *)CFBridgingRetain(newDisposable);
+	RACDisposable *existingDisposable;
+	BOOL alreadyDisposed;
+
+	OSSpinLockLock(&_spinLock);
+	alreadyDisposed = _disposed;
+	if (!alreadyDisposed) {
+		existingDisposable = _disposable;
+		_disposable = newDisposable;
 	}
+	OSSpinLockUnlock(&_spinLock);
 
-	void *existingDisposablePtr;
-	// Keep trying while we're not disposed.
-	while ((existingDisposablePtr = _disposablePtr) != NULL) {
-		if (!OSAtomicCompareAndSwapPtrBarrier(existingDisposablePtr, newDisposablePtr, &_disposablePtr)) {
-			continue;
-		}
-
-		// Return nil if _disposablePtr was set to self. Otherwise, release
-		// the old value and return it as an object.
-		if (existingDisposablePtr == selfPtr) {
-			return nil;
-		} else {
-			return CFBridgingRelease(existingDisposablePtr);
-		}
+	if (alreadyDisposed) {
+		[newDisposable dispose];
+		return nil;
 	}
 
-	// At this point, we've found out that we were already disposed.
-	[newDisposable dispose];
-
-	// Failed to swap, clean up the ownership we took prior to the swap.
-	if (newDisposable != nil) {
-		CFRelease(newDisposablePtr);
-	}
-
-	return nil;
+	return existingDisposable;
 }
 
 #pragma mark Disposal
 
 - (void)dispose {
-	void *existingDisposablePtr;
-
-	while ((existingDisposablePtr = _disposablePtr) != NULL) {
-		if (OSAtomicCompareAndSwapPtrBarrier(existingDisposablePtr, NULL, &_disposablePtr)) {
-			if (existingDisposablePtr != (__bridge void *)self) {
-				RACDisposable *existingDisposable = CFBridgingRelease(existingDisposablePtr);
-				[existingDisposable dispose];
-			}
+	RACDisposable *existingDisposable;
 
-			break;
-		}
+	OSSpinLockLock(&_spinLock);
+	if (!_disposed) {
+		existingDisposable = _disposable;
+		_disposed = YES;
+		_disposable = nil;
 	}
+	OSSpinLockUnlock(&_spinLock);
+	
+	[existingDisposable dispose];
 }
 
 @end

ReactiveCocoaFramework/ReactiveCocoaTests/RACSerialDisposableSpec.m

@@ -134,4 +134,26 @@
 	expect(weakInnerDisposable).to.beNil();
 });
 
+it(@"should not crash when racing between swapInDisposable and disposable", ^{
+	__block BOOL stop = NO;
+	dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (long long)(0.1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
+		stop = YES;
+	});
+
+	RACSerialDisposable *serialDisposable =  [[RACSerialDisposable alloc] init];
+	dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+		while (!stop) {
+			[serialDisposable swapInDisposable:[RACDisposable disposableWithBlock:^{}]];
+		}
+	});
+
+	dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+		while (!stop) {
+			[serialDisposable disposable];
+		}
+	});
+
+	expect(stop).will.beTruthy();
+});
+
 SpecEnd