Fix a problem where both finished and failed delegate methods could in some circ…

…umstances be called for the same request Thanks to Mike DeSaro for catching this nasty bug!

Fix a problem where both finished and failed delegate methods could in some circ…
…umstances be called for the same request Thanks to Mike DeSaro for catching this nasty bug!
Ben Copsey
Commit c73fb11abc5d4839cb38fa5ebf55319fade5439a c73fb11a 1 parent 83231507
Showing 5 changed files with 97 additions and 49 deletions
Classes/ASIHTTPRequest.m
Classes/Tests/ASIHTTPRequestTests.h
Classes/Tests/ASIHTTPRequestTests.m
Classes/Tests/ASINetworkQueueTests.h
Classes/Tests/ASINetworkQueueTests.m
--- a/Classes/ASIHTTPRequest.m
View file @c73fb11
+++ b/Classes/ASIHTTPRequest.m
View file @c73fb11
@@ -263,6 +263,10 @@ static NSError *ASITooMuchRedirectionError;
 - (void)cancel
 {
+	// Request may already be complete
+	if ([self complete] || [self isCancelled]) {
+		return;
+	}
 	[self failWithError:ASIRequestCancelledError];
 	[super cancel];
 	[self cancelLoad];
@@ -897,7 +901,7 @@ static NSError *ASITooMuchRedirectionError;
 // 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 @@ static NSError *ASITooMuchRedirectionError;
 {
 	[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
+		// Let the queue know something went wrong
-		if ([self mainRequest]) {
+		if ([queue respondsToSelector:@selector(requestDidFail:)]) {
-			ASIHTTPRequest *mRequest = [self mainRequest];
+			[queue performSelectorOnMainThread:@selector(requestDidFail:) withObject:self waitUntilDone:[NSThread isMainThread]];		
-			[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 {
+		// Let the delegate know something went wrong
-			[self setError:theError];
+		if (didFailSelector && [delegate respondsToSelector:didFailSelector]) {
-			
+			[delegate performSelectorOnMainThread:didFailSelector withObject:self waitUntilDone:[NSThread isMainThread]];	
-			// 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]];	
-			}
 		}
-
 	}
 }
--- a/Classes/Tests/ASIHTTPRequestTests.h
View file @c73fb11
+++ b/Classes/Tests/ASIHTTPRequestTests.h
View file @c73fb11
@@ -37,4 +37,5 @@
 - (void)testTooMuchRedirection;
 - (void)testRedirectToNewDomain;
 - (void)test303Redirect;
+
 @end
--- a/Classes/Tests/ASIHTTPRequestTests.m
View file @c73fb11
+++ b/Classes/Tests/ASIHTTPRequestTests.m
View file @c73fb11
@@ -680,4 +680,6 @@
 	GHAssertTrue(success,@"Failed to use GET on new URL");
 }
+
+
 @end
--- a/Classes/Tests/ASINetworkQueueTests.h
View file @c73fb11
+++ b/Classes/Tests/ASINetworkQueueTests.h
View file @c73fb11
@@ -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
--- a/Classes/Tests/ASINetworkQueueTests.m
View file @c73fb11
+++ b/Classes/Tests/ASINetworkQueueTests.m
View file @c73fb11
@@ -19,7 +19,7 @@
 	complete = NO;
 	progress = 0;
-	networkQueue = [[ASINetworkQueue queue] retain];
+	ASINetworkQueue *networkQueue = [ASINetworkQueue queue];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setDelegate:self];
 	[networkQueue setShowAccurateProgress:NO];
@@ -73,9 +73,6 @@
 	success = (progress > 0.95);
 	GHAssertTrue(success,@"Failed to increment progress properly");
-	
-	[networkQueue release];
-	
 }
 - (void)testUploadProgress
@@ -83,7 +80,7 @@
 	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 @@
 	success = (progress > 0.95);
 	GHAssertTrue(success,@"Failed to increment progress properly");
-	[networkQueue release];
-	
 }
@@ -153,7 +148,7 @@
 {
 	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 @@
 {
 	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 @@
 	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 @@
 	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 @@
 	error = [request error];
 	GHAssertNil(error,@"Failed to use authentication in a queue");	
-	[networkQueue release];
 }
@@ -345,7 +338,7 @@
 {
     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 @@
 	// 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 @@
 	}
 	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 @@
 	// 5 seconds is up, let's tell the queue to stop
 	[networkQueue cancelAllOperations];
-	[networkQueue release];
+	networkQueue = [ASINetworkQueue queue];
-	networkQueue = [[ASINetworkQueue alloc] init];
 	[networkQueue setDownloadProgressDelegate:self];
 	[networkQueue setShowAccurateProgress:YES];
 	[networkQueue setDelegate:self];
@@ -435,13 +426,12 @@
 	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 @@
 	GHAssertTrue(success,@"Temporary download file should have been deleted");		
 	timeoutTimer = nil;
-	[networkQueue release];	
 }
@@ -480,5 +469,55 @@
 }
+// 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