From 680fcd5071cc890189aa8848ffd87efcf11f010e Mon Sep 17 00:00:00 2001 From: Mattt Thompson Date: Thu, 8 Dec 2011 12:23:58 -0600 Subject: [PATCH 1/4] First iteration of thread-safety on AFURLConnectionOperation, using a combination of NSRecursiveLock and common sense --- AFNetworking/AFURLConnectionOperation.h | 4 + AFNetworking/AFURLConnectionOperation.m | 174 ++++++++++++++---------- 2 files changed, 103 insertions(+), 75 deletions(-) diff --git a/AFNetworking/AFURLConnectionOperation.h b/AFNetworking/AFURLConnectionOperation.h index 600981c..a88809e 100644 --- a/AFNetworking/AFURLConnectionOperation.h +++ b/AFNetworking/AFURLConnectionOperation.h @@ -75,6 +75,10 @@ extern NSString * const AFNetworkingOperationDidFinishNotification; */ @interface AFURLConnectionOperation : NSOperation { @private + unsigned short _state; + BOOL _cancelled; + NSRecursiveLock *_lock; + NSSet *_runLoopModes; NSURLConnection *_connection; diff --git a/AFNetworking/AFURLConnectionOperation.m b/AFNetworking/AFURLConnectionOperation.m index bc8a71e..32579fa 100644 --- a/AFNetworking/AFURLConnectionOperation.m +++ b/AFNetworking/AFURLConnectionOperation.m @@ -22,14 +22,18 @@ #import "AFURLConnectionOperation.h" -static NSUInteger const kAFHTTPMinimumInitialDataCapacity = 1024; -static NSUInteger const kAFHTTPMaximumInitialDataCapacity = 1024 * 1024 * 8; - typedef enum { AFHTTPOperationReadyState = 1, AFHTTPOperationExecutingState = 2, AFHTTPOperationFinishedState = 3, -} AFOperationState; +} _AFOperationState; + +typedef unsigned short AFOperationState; + +static NSUInteger const kAFHTTPMinimumInitialDataCapacity = 1024; +static NSUInteger const kAFHTTPMaximumInitialDataCapacity = 1024 * 1024 * 8; + +static NSString * const kAFNetworkingLockName = @"com.alamofire.networking.operation.lock"; NSString * const AFNetworkingErrorDomain = @"com.alamofire.networking.error"; @@ -51,9 +55,35 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { } } +static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperationState toState, BOOL isCancelled) { + switch (fromState) { + case AFHTTPOperationReadyState: + switch (toState) { + case AFHTTPOperationExecutingState: + return YES; + case AFHTTPOperationFinishedState: + return isCancelled; + default: + return NO; + } + case AFHTTPOperationExecutingState: + switch (toState) { + case AFHTTPOperationFinishedState: + return YES; + default: + return NO; + } + case AFHTTPOperationFinishedState: + return NO; + default: + return YES; + } +} + @interface AFURLConnectionOperation () @property (readwrite, nonatomic, assign) AFOperationState state; @property (readwrite, nonatomic, assign, getter = isCancelled) BOOL cancelled; +@property (readwrite, nonatomic, retain) NSRecursiveLock *lock; @property (readwrite, nonatomic, assign) NSURLConnection *connection; @property (readwrite, nonatomic, retain) NSURLRequest *request; @property (readwrite, nonatomic, retain) NSURLResponse *response; @@ -65,7 +95,6 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { @property (readwrite, nonatomic, copy) AFURLConnectionOperationProgressBlock uploadProgress; @property (readwrite, nonatomic, copy) AFURLConnectionOperationProgressBlock downloadProgress; -- (BOOL)shouldTransitionToState:(AFOperationState)state; - (void)operationDidStart; - (void)finish; @end @@ -86,6 +115,7 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { @synthesize outputStream = _outputStream; @synthesize uploadProgress = _uploadProgress; @synthesize downloadProgress = _downloadProgress; +@synthesize lock = _lock; + (void)networkRequestThreadEntryPoint:(id)__unused object { do { @@ -113,6 +143,9 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { return nil; } + self.lock = [[[NSRecursiveLock alloc] init] autorelease]; + self.lock.name = kAFNetworkingLockName; + self.runLoopModes = [NSSet setWithObject:NSRunLoopCommonModes]; self.request = urlRequest; @@ -123,6 +156,8 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { } - (void)dealloc { + [_lock release]; + [_runLoopModes release]; [_request release]; @@ -132,7 +167,12 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { [_responseData release]; [_responseString release]; [_dataAccumulator release]; - [_outputStream release]; _outputStream = nil; + + if (_outputStream) { + [_outputStream close]; + [_outputStream release]; + _outputStream = nil; + } [_uploadProgress release]; [_downloadProgress release]; @@ -141,6 +181,7 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { } - (void)setCompletionBlock:(void (^)(void))block { + [self.lock lock]; if (!block) { [super setCompletionBlock:nil]; } else { @@ -150,6 +191,7 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { [_blockSelf setCompletionBlock:nil]; }]; } + [self.lock unlock]; } - (NSInputStream *)inputStream { @@ -171,65 +213,41 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { } - (void)setState:(AFOperationState)state { - if (![self shouldTransitionToState:state]) { - return; - } - - NSString *oldStateKey = AFKeyPathFromOperationState(self.state); - NSString *newStateKey = AFKeyPathFromOperationState(state); - - [self willChangeValueForKey:newStateKey]; - [self willChangeValueForKey:oldStateKey]; - _state = state; - [self didChangeValueForKey:oldStateKey]; - [self didChangeValueForKey:newStateKey]; - - switch (state) { - case AFHTTPOperationExecutingState: - [[NSNotificationCenter defaultCenter] postNotificationName:AFNetworkingOperationDidStartNotification object:self]; - break; - case AFHTTPOperationFinishedState: - [[NSNotificationCenter defaultCenter] postNotificationName:AFNetworkingOperationDidFinishNotification object:self]; - break; - default: - break; - } -} - -- (BOOL)shouldTransitionToState:(AFOperationState)state { - switch (self.state) { - case AFHTTPOperationReadyState: - switch (state) { - case AFHTTPOperationExecutingState: - return YES; - default: - return NO; - } - case AFHTTPOperationExecutingState: - switch (state) { - case AFHTTPOperationFinishedState: - return YES; - default: - return NO; - } - case AFHTTPOperationFinishedState: - return NO; - default: - return YES; + [self.lock lock]; + if (AFStateTransitionIsValid(self.state, state, [self isCancelled])) { + NSString *oldStateKey = AFKeyPathFromOperationState(self.state); + NSString *newStateKey = AFKeyPathFromOperationState(state); + + [self willChangeValueForKey:newStateKey]; + [self willChangeValueForKey:oldStateKey]; + _state = state; + [self didChangeValueForKey:oldStateKey]; + [self didChangeValueForKey:newStateKey]; + + switch (state) { + case AFHTTPOperationExecutingState: + [[NSNotificationCenter defaultCenter] postNotificationName:AFNetworkingOperationDidStartNotification object:self]; + break; + case AFHTTPOperationFinishedState: + [[NSNotificationCenter defaultCenter] postNotificationName:AFNetworkingOperationDidFinishNotification object:self]; + break; + default: + break; + } } + [self.lock unlock]; } - (void)setCancelled:(BOOL)cancelled { + [self.lock lock]; [self willChangeValueForKey:@"isCancelled"]; _cancelled = cancelled; [self didChangeValueForKey:@"isCancelled"]; - - if ([self isCancelled]) { - self.state = AFHTTPOperationFinishedState; - } + [self.lock unlock]; } - (NSString *)responseString { + [self.lock lock]; if (!_responseString && self.response && self.responseData) { NSStringEncoding textEncoding = NSUTF8StringEncoding; if (self.response.textEncodingName) { @@ -238,6 +256,7 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { self.responseString = [[[NSString alloc] initWithData:self.responseData encoding:textEncoding] autorelease]; } + [self.lock unlock]; return _responseString; } @@ -271,20 +290,21 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { } - (void)operationDidStart { + [self.lock lock]; if ([self isCancelled]) { [self finish]; - return; + } else { + self.connection = [[[NSURLConnection alloc] initWithRequest:self.request delegate:self startImmediately:NO] autorelease]; + + NSRunLoop *runLoop = [NSRunLoop currentRunLoop]; + for (NSString *runLoopMode in self.runLoopModes) { + [self.connection scheduleInRunLoop:runLoop forMode:runLoopMode]; + [self.outputStream scheduleInRunLoop:runLoop forMode:runLoopMode]; + } + + [self.connection start]; } - - self.connection = [[[NSURLConnection alloc] initWithRequest:self.request delegate:self startImmediately:NO] autorelease]; - - NSRunLoop *runLoop = [NSRunLoop currentRunLoop]; - for (NSString *runLoopMode in self.runLoopModes) { - [self.connection scheduleInRunLoop:runLoop forMode:runLoopMode]; - [self.outputStream scheduleInRunLoop:runLoop forMode:runLoopMode]; - } - - [self.connection start]; + [self.lock unlock]; } - (void)finish { @@ -292,15 +312,19 @@ static inline NSString * AFKeyPathFromOperationState(AFOperationState state) { } - (void)cancel { - if ([self isFinished]) { - return; + [self.lock lock]; + if (![self isFinished] && ![self isCancelled]) { + [super cancel]; + + self.cancelled = YES; + + [self.connection cancel]; + + // We must send this delegate protcol message ourselves since the above [self.connection cancel] causes the connection to never send another message to its delegate. + NSDictionary *userInfo = [NSDictionary dictionaryWithObject:[self.request URL] forKey:NSURLErrorFailingURLErrorKey]; + [self performSelector:@selector(connection:didFailWithError:) withObject:self.connection withObject:[NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorCancelled userInfo:userInfo]]; } - - [super cancel]; - - self.cancelled = YES; - - [self.connection cancel]; + [self.lock unlock]; } #pragma mark - NSURLConnectionDelegate @@ -361,7 +385,7 @@ didReceiveResponse:(NSURLResponse *)response - (void)connection:(NSURLConnection *)__unused connection didFailWithError:(NSError *)error -{ +{ self.error = error; if (self.outputStream) { From a466c27886691680ca879ac282cca4acf56e28df Mon Sep 17 00:00:00 2001 From: Mattt Thompson Date: Thu, 8 Dec 2011 12:42:59 -0600 Subject: [PATCH 2/4] Adding @try/@catch with autorelease pool for exceptions Wrapping -start in lock --- AFNetworking/AFURLConnectionOperation.m | 41 ++++++++++++++++--------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/AFNetworking/AFURLConnectionOperation.m b/AFNetworking/AFURLConnectionOperation.m index 32579fa..4e762e5 100644 --- a/AFNetworking/AFURLConnectionOperation.m +++ b/AFNetworking/AFURLConnectionOperation.m @@ -119,9 +119,18 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat + (void)networkRequestThreadEntryPoint:(id)__unused object { do { - NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init]; - [[NSRunLoop currentRunLoop] run]; - [pool drain]; + NSAutoreleasePool *exceptionPool = [[NSAutoreleasePool alloc] init]; + NSException *caughtException = nil; + @try { + NSAutoreleasePool *runLoopPool = [[NSAutoreleasePool alloc] init]; + [[NSRunLoop currentRunLoop] run]; + [runLoopPool drain]; + } + @catch(NSException *e) { caughtException = e; } + if(caughtException) { + NSLog(NSLocalizedString(@"Unhandled exception on %@ networking thread: %@, userInfo: %@", nil), NSStringFromClass([self class]), caughtException, [caughtException userInfo]); + } + [exceptionPool drain]; } while (YES); } @@ -279,14 +288,14 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat return YES; } -- (void)start { - if (![self isReady]) { - return; +- (void)start { + [self.lock lock]; + if ([self isReady]) { + self.state = AFHTTPOperationExecutingState; + + [self performSelector:@selector(operationDidStart) onThread:[[self class] networkRequestThread] withObject:nil waitUntilDone:NO modes:[self.runLoopModes allObjects]]; } - - self.state = AFHTTPOperationExecutingState; - - [self performSelector:@selector(operationDidStart) onThread:[[self class] networkRequestThread] withObject:nil waitUntilDone:YES modes:[self.runLoopModes allObjects]]; + [self.lock unlock]; } - (void)operationDidStart { @@ -318,11 +327,13 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat self.cancelled = YES; - [self.connection cancel]; - - // We must send this delegate protcol message ourselves since the above [self.connection cancel] causes the connection to never send another message to its delegate. - NSDictionary *userInfo = [NSDictionary dictionaryWithObject:[self.request URL] forKey:NSURLErrorFailingURLErrorKey]; - [self performSelector:@selector(connection:didFailWithError:) withObject:self.connection withObject:[NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorCancelled userInfo:userInfo]]; + if (self.connection) { + [self.connection cancel]; + + // We must send this delegate protcol message ourselves since the above [self.connection cancel] causes the connection to never send another message to its delegate. + NSDictionary *userInfo = [NSDictionary dictionaryWithObject:[self.request URL] forKey:NSURLErrorFailingURLErrorKey]; + [self performSelector:@selector(connection:didFailWithError:) withObject:self.connection withObject:[NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorCancelled userInfo:userInfo]]; + } } [self.lock unlock]; } From 1c606919fc8e1efef8c783b5963196bb1111218a Mon Sep 17 00:00:00 2001 From: Mattt Thompson Date: Thu, 8 Dec 2011 12:43:39 -0600 Subject: [PATCH 3/4] UIImageView shouldn't set image to placeholder image whenever an old image request comes in --- AFNetworking/UIImageView+AFNetworking.m | 2 -- 1 file changed, 2 deletions(-) diff --git a/AFNetworking/UIImageView+AFNetworking.m b/AFNetworking/UIImageView+AFNetworking.m index 69b9653..b02113b 100644 --- a/AFNetworking/UIImageView+AFNetworking.m +++ b/AFNetworking/UIImageView+AFNetworking.m @@ -108,8 +108,6 @@ static char kAFImageRequestOperationObjectKey; if ([[urlRequest URL] isEqual:[[self.af_imageRequestOperation request] URL]]) { self.image = responseObject; - } else { - self.image = placeholderImage; } [[AFImageCache sharedImageCache] cacheImageData:operation.responseData forURL:[urlRequest URL] cacheName:nil]; From 68c572ae32b320be2b96ac010c002a3f080c9e03 Mon Sep 17 00:00:00 2001 From: Mattt Thompson Date: Thu, 8 Dec 2011 13:21:23 -0600 Subject: [PATCH 4/4] Refactoring -cancel and -isCancelled --- AFNetworking/AFURLConnectionOperation.m | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/AFNetworking/AFURLConnectionOperation.m b/AFNetworking/AFURLConnectionOperation.m index 4e762e5..0ef5b55 100644 --- a/AFNetworking/AFURLConnectionOperation.m +++ b/AFNetworking/AFURLConnectionOperation.m @@ -82,7 +82,6 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat @interface AFURLConnectionOperation () @property (readwrite, nonatomic, assign) AFOperationState state; -@property (readwrite, nonatomic, assign, getter = isCancelled) BOOL cancelled; @property (readwrite, nonatomic, retain) NSRecursiveLock *lock; @property (readwrite, nonatomic, assign) NSURLConnection *connection; @property (readwrite, nonatomic, retain) NSURLRequest *request; @@ -101,7 +100,6 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat @implementation AFURLConnectionOperation @synthesize state = _state; -@synthesize cancelled = _cancelled; @synthesize connection = _connection; @synthesize runLoopModes = _runLoopModes; @synthesize request = _request; @@ -247,14 +245,6 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat [self.lock unlock]; } -- (void)setCancelled:(BOOL)cancelled { - [self.lock lock]; - [self willChangeValueForKey:@"isCancelled"]; - _cancelled = cancelled; - [self didChangeValueForKey:@"isCancelled"]; - [self.lock unlock]; -} - - (NSString *)responseString { [self.lock lock]; if (!_responseString && self.response && self.responseData) { @@ -284,6 +274,10 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat return self.state == AFHTTPOperationFinishedState; } +- (BOOL)isCancelled { + return _cancelled; +} + - (BOOL)isConcurrent { return YES; } @@ -325,8 +319,8 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat if (![self isFinished] && ![self isCancelled]) { [super cancel]; - self.cancelled = YES; - + [self willChangeValueForKey:@"isCancelled"]; + _cancelled = YES; if (self.connection) { [self.connection cancel]; @@ -334,6 +328,7 @@ static inline BOOL AFStateTransitionIsValid(AFOperationState fromState, AFOperat NSDictionary *userInfo = [NSDictionary dictionaryWithObject:[self.request URL] forKey:NSURLErrorFailingURLErrorKey]; [self performSelector:@selector(connection:didFailWithError:) withObject:self.connection withObject:[NSError errorWithDomain:NSURLErrorDomain code:NSURLErrorCancelled userInfo:userInfo]]; } + [self didChangeValueForKey:@"isCancelled"]; } [self.lock unlock]; }