Fix a problem where both finished and failed delegate methods could i…

…n some circumstances be called for the same request Thanks to Mike DeSaro for catching this nasty bug!
glebd · Jul 16, 2009 · c73fb11 · c73fb11
1 parent 8323150
commit c73fb11
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 49 deletions.
diff --git a/Classes/ASIHTTPRequest.m b/Classes/ASIHTTPRequest.m
@@ -263,6 +263,10 @@ - (BOOL)isFinished
 
 - (void)cancel
 {
+	// Request may already be complete
+	if ([self complete] || [self isCancelled]) {
+		return;
+	}
 	[self failWithError:ASIRequestCancelledError];
 	[super cancel];
 	[self cancelLoad];
@@ -897,7 +901,7 @@ + (void)setProgress:(double)progress forProgressIndicator:(id)indicator
 // If you do this, don't forget to call [super requestFinished] to let the queue / delegate know we're done
 - (void)requestFinished
 {
-	if ([self isCancelled] || [self mainRequest]) {
+	if ([self error] || [self mainRequest]) {
 		return;
 	}
 	// Let the queue know we are done
@@ -917,36 +921,32 @@ - (void)failWithError:(NSError *)theError
 {
 	[self setComplete:YES];
 
-	if ([self isCancelled]) {
+	if ([self isCancelled] || [self error]) {
 		return;
 	}
 
-	if (!error) {
+	// If this is a HEAD request created by an ASINetworkQueue or compatible queue delegate, make the main request fail
+	if ([self mainRequest]) {
+		ASIHTTPRequest *mRequest = [self mainRequest];
+		[mRequest setError:theError];
+
+		// Let the queue know something went wrong
+		if ([queue respondsToSelector:@selector(requestDidFail:)]) {
+			[queue performSelectorOnMainThread:@selector(requestDidFail:) withObject:mRequest waitUntilDone:[NSThread isMainThread]];		
+		}
+
+	} else {
+		[self setError:theError];
 
-		// If this is a HEAD request created by an ASINetworkQueue or compatible queue delegate, make the main request fail
-		if ([self mainRequest]) {
-			ASIHTTPRequest *mRequest = [self mainRequest];
-			[mRequest setError:theError];
-
-			// Let the queue know something went wrong
-			if ([queue respondsToSelector:@selector(requestDidFail:)]) {
-				[queue performSelectorOnMainThread:@selector(requestDidFail:) withObject:mRequest waitUntilDone:[NSThread isMainThread]];		
-			}
+		// Let the queue know something went wrong
+		if ([queue respondsToSelector:@selector(requestDidFail:)]) {
+			[queue performSelectorOnMainThread:@selector(requestDidFail:) withObject:self waitUntilDone:[NSThread isMainThread]];		
+		}
 
-		} else {
-			[self setError:theError];
-
-			// Let the queue know something went wrong
-			if ([queue respondsToSelector:@selector(requestDidFail:)]) {
-				[queue performSelectorOnMainThread:@selector(requestDidFail:) withObject:self waitUntilDone:[NSThread isMainThread]];		
-			}
-
-			// Let the delegate know something went wrong
-			if (didFailSelector && [delegate respondsToSelector:didFailSelector]) {
-				[delegate performSelectorOnMainThread:didFailSelector withObject:self waitUntilDone:[NSThread isMainThread]];	
-			}
+		// Let the delegate know something went wrong
+		if (didFailSelector && [delegate respondsToSelector:didFailSelector]) {
+			[delegate performSelectorOnMainThread:didFailSelector withObject:self waitUntilDone:[NSThread isMainThread]];	
 		}
-
 	}
 }
 

diff --git a/Classes/Tests/ASIHTTPRequestTests.h b/Classes/Tests/ASIHTTPRequestTests.h
@@ -37,4 +37,5 @@
 - (void)testTooMuchRedirection;
 - (void)testRedirectToNewDomain;
 - (void)test303Redirect;
+
 @end
diff --git a/Classes/Tests/ASIHTTPRequestTests.m b/Classes/Tests/ASIHTTPRequestTests.m
@@ -680,4 +680,6 @@ - (void)test303Redirect
 	GHAssertTrue(success,@"Failed to use GET on new URL");
 }
 
+
+
 @end
diff --git a/Classes/Tests/ASINetworkQueueTests.h b/Classes/Tests/ASINetworkQueueTests.h
@@ -13,15 +13,17 @@
 #endif
 
 @class ASIHTTPRequest;
-@class ASINetworkQueue;
 
 @interface ASINetworkQueueTests : GHTestCase {
 	ASIHTTPRequest *requestThatShouldFail;
-	ASINetworkQueue *networkQueue;
 	BOOL complete;
 	BOOL request_didfail;
 	BOOL request_succeeded;
 	float progress;
+
+	NSOperationQueue *immediateCancelQueue;
+	NSMutableArray *failedRequests;
+	NSMutableArray *finishedRequests;
 }
 
 - (void)testFailure;
@@ -31,7 +33,11 @@
 - (void)testProgressWithAuthentication;
 - (void)testWithNoListener;
 - (void)testPartialResume;
+- (void)testImmediateCancel;
 
 - (void)setProgress:(float)newProgress;
 
+@property (retain) NSOperationQueue *immediateCancelQueue;
+@property (retain) NSMutableArray *failedRequests;
+@property (retain) NSMutableArray *finishedRequests;
 @end
diff --git a/Classes/Tests/ASINetworkQueueTests.m b/Classes/Tests/ASINetworkQueueTests.m
@@ -19,7 +19,7 @@ - (void)testProgress
 	complete = NO;
 	progress = 0;
 
-	networkQueue = [[ASINetworkQueue queue] retain];
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setDelegate:self];
 	[networkQueue setShowAccurateProgress:NO];
@@ -73,17 +73,14 @@ - (void)testProgress
 	success = (progress > 0.95);
 	GHAssertTrue(success,@"Failed to increment progress properly");
 
-
-	[networkQueue release];
-
 }
 
 - (void)testUploadProgress
 {
 	complete = NO;
 	progress = 0;
 
-	networkQueue = [[ASINetworkQueue alloc] init];
+	ASINetworkQueue *networkQueue = [[[ASINetworkQueue alloc] init] autorelease];
 	[networkQueue setUploadProgressDelegate:self];
 	[networkQueue setDelegate:self];
 	[networkQueue setShowAccurateProgress:NO];
@@ -135,8 +132,6 @@ - (void)testUploadProgress
 	success = (progress > 0.95);
 	GHAssertTrue(success,@"Failed to increment progress properly");
 
-	[networkQueue release];
-
 }
 
 
@@ -153,7 +148,7 @@ - (void)testFailure
 {
 	complete = NO;
 
-	networkQueue = [[ASINetworkQueue alloc] init];
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDelegate:self];
 	[networkQueue setRequestDidFailSelector:@selector(requestFailed:)];
 	[networkQueue setQueueDidFinishSelector:@selector(queueFinished:)];
@@ -224,7 +219,7 @@ - (void)testFailureCancelsOtherRequests
 {
 	complete = NO;
 
-	networkQueue = [[ASINetworkQueue alloc] init];
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDelegate:self];
 	[networkQueue setRequestDidFailSelector:@selector(requestFailedCancellingOthers:)];
 	[networkQueue setQueueDidFinishSelector:@selector(queueFinished:)];	
@@ -278,7 +273,7 @@ - (void)testProgressWithAuthentication
 	complete = NO;
 	progress = 0;
 
-	networkQueue = [[ASINetworkQueue alloc] init];
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setDelegate:self];
 	[networkQueue setShowAccurateProgress:YES];
@@ -299,11 +294,10 @@ - (void)testProgressWithAuthentication
 
 	NSError *error = [request error];
 	GHAssertNotNil(error,@"The HEAD request failed, but it didn't tell the main request to fail");	
-	[networkQueue release];
 
 	complete = NO;
 	progress = 0;	
-	networkQueue = [[ASINetworkQueue alloc] init];
+	networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setDelegate:self];
 	[networkQueue setShowAccurateProgress:YES];
@@ -322,7 +316,6 @@ - (void)testProgressWithAuthentication
 
 	error = [request error];
 	GHAssertNil(error,@"Failed to use authentication in a queue");	
-	[networkQueue release];
 
 }
 
@@ -345,7 +338,7 @@ - (void)testWithNoListener
 {
     request_succeeded = NO;
     request_didfail = NO;
-	networkQueue = [[ASINetworkQueue alloc] init];
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setDelegate:self];
 	[networkQueue setShowAccurateProgress:YES];
@@ -364,7 +357,6 @@ - (void)testWithNoListener
 	// This test may fail if you are using a proxy and it returns a page when you try to connect to a bad port.
 	GHAssertTrue(!request_succeeded && request_didfail,@"Request to resource without listener succeeded but should have failed");
 
-	[networkQueue release];
 }
 
 - (void)testPartialResume
@@ -383,7 +375,7 @@ - (void)testPartialResume
 	}
 
 	NSURL *downloadURL = [NSURL URLWithString:@"http://trails-network.net/Downloads/MemexTrails_1.0b1.zip"];
-	networkQueue = [[ASINetworkQueue alloc] init];	
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];	
 
 	ASIHTTPRequest *request = [[[ASIHTTPRequest alloc] initWithURL:downloadURL] autorelease];
 	[request setDownloadDestinationPath:downloadPath];
@@ -402,8 +394,7 @@ - (void)testPartialResume
 	// 5 seconds is up, let's tell the queue to stop
 	[networkQueue cancelAllOperations];
 
-	[networkQueue release];
-	networkQueue = [[ASINetworkQueue alloc] init];
+	networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setShowAccurateProgress:YES];
 	[networkQueue setDelegate:self];
@@ -435,13 +426,12 @@ - (void)testPartialResume
 	success = (progress > 0.95);
 	GHAssertTrue(success,@"Failed to increment progress properly");
 
-	[networkQueue release];	
-
+
 
 	//Test the temporary file cleanup
 	complete = NO;
 	progress = 0;
-	networkQueue = [[ASINetworkQueue alloc] init];	
+	networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setShowAccurateProgress:YES];
 	[networkQueue setDelegate:self];
@@ -470,7 +460,6 @@ - (void)testPartialResume
 	GHAssertTrue(success,@"Temporary download file should have been deleted");		
 
 	timeoutTimer = nil;
-	[networkQueue release];	
 
 }
 
@@ -480,5 +469,55 @@ - (void)stopQueue:(id)sender
 }
 
 
+// Not strictly an ASINetworkQueue test, but queue related
+// As soon as one request finishes or fails, we'll cancel the others and ensure that no requests are both finished and failed
+- (void)testImmediateCancel
+{
+	[self setFailedRequests:[[[NSMutableArray alloc] init] autorelease]];
+	[self setFinishedRequests:[[[NSMutableArray alloc] init] autorelease]];
+	[self setImmediateCancelQueue:[[[NSOperationQueue alloc] init] autorelease]];
+	int i;
+	for (i=0; i<100; i++) {
+		ASIHTTPRequest *request = [ASIHTTPRequest requestWithURL:[NSURL URLWithString:@"http://asi"]];
+		[request setDelegate:self];
+		[request setDidFailSelector:@selector(immediateCancelFail:)];
+		[request setDidFinishSelector:@selector(immediateCancelFinish:)];
+		[[self immediateCancelQueue] addOperation:request];
+	}
+
+}
+
+- (void)immediateCancelFail:(ASIHTTPRequest *)request
+{
+	[[self immediateCancelQueue] cancelAllOperations];
+	if ([[self failedRequests] containsObject:request]) {
+		GHFail(@"A request called its fail delegate method twice");
+	}
+	if ([[self finishedRequests] containsObject:request]) {
+		GHFail(@"A request that had already finished called its fail delegate method");
+	}
+	[[self failedRequests] addObject:request];
+	if ([[self failedRequests] count]+[[self finishedRequests] count] > 100) {
+		GHFail(@"We got more than 100 delegate fail/finish calls - this shouldn't happen!");
+	}
+}
+
+- (void)immediateCancelFinish:(ASIHTTPRequest *)request
+{
+	[[self immediateCancelQueue] cancelAllOperations];
+	if ([[self finishedRequests] containsObject:request]) {
+		GHFail(@"A request called its finish delegate method twice");
+	}
+	if ([[self failedRequests] containsObject:request]) {
+		GHFail(@"A request that had already failed called its finish delegate method");
+	}
+	[[self finishedRequests] addObject:request];
+	if ([[self failedRequests] count]+[[self finishedRequests] count] > 100) {
+		GHFail(@"We got more than 100 delegate fail/finish calls - this shouldn't happen!");
+	}
+}
 
+@synthesize immediateCancelQueue;
+@synthesize failedRequests;
+@synthesize finishedRequests;
 @end