Fix potential crasher that could have been triggered when requests are cancelled
Showing
3 changed files
with
39 additions
and
3 deletions
| @@ -668,6 +668,9 @@ static NSLock *sessionCookiesLock = nil; | @@ -668,6 +668,9 @@ static NSLock *sessionCookiesLock = nil; | ||
| 668 | 668 | ||
| 669 | NSDate *now = [NSDate date]; | 669 | NSDate *now = [NSDate date]; |
| 670 | 670 | ||
| 671 | + // We won't let the request cancel until we're done with this cycle of the loop | ||
| 672 | + [[self cancelledLock] lock]; | ||
| 673 | + | ||
| 671 | // See if we need to timeout | 674 | // See if we need to timeout |
| 672 | if (lastActivityTime && timeOutSeconds > 0 && [now timeIntervalSinceDate:lastActivityTime] > timeOutSeconds) { | 675 | if (lastActivityTime && timeOutSeconds > 0 && [now timeIntervalSinceDate:lastActivityTime] > timeOutSeconds) { |
| 673 | 676 | ||
| @@ -676,6 +679,7 @@ static NSLock *sessionCookiesLock = nil; | @@ -676,6 +679,7 @@ static NSLock *sessionCookiesLock = nil; | ||
| 676 | // This workaround prevents erroneous timeouts in low bandwidth situations (eg iPhone) | 679 | // This workaround prevents erroneous timeouts in low bandwidth situations (eg iPhone) |
| 677 | if (totalBytesSent || postLength <= uploadBufferSize || (uploadBufferSize > 0 && totalBytesSent > uploadBufferSize)) { | 680 | if (totalBytesSent || postLength <= uploadBufferSize || (uploadBufferSize > 0 && totalBytesSent > uploadBufferSize)) { |
| 678 | [self failWithError:ASIRequestTimedOutError]; | 681 | [self failWithError:ASIRequestTimedOutError]; |
| 682 | + [[self cancelledLock] unlock]; | ||
| 679 | [self cancelLoad]; | 683 | [self cancelLoad]; |
| 680 | [self setComplete:YES]; | 684 | [self setComplete:YES]; |
| 681 | break; | 685 | break; |
| @@ -684,6 +688,7 @@ static NSLock *sessionCookiesLock = nil; | @@ -684,6 +688,7 @@ static NSLock *sessionCookiesLock = nil; | ||
| 684 | 688 | ||
| 685 | // Do we need to redirect? | 689 | // Do we need to redirect? |
| 686 | if ([self needsRedirect]) { | 690 | if ([self needsRedirect]) { |
| 691 | + [[self cancelledLock] unlock]; | ||
| 687 | [self cancelLoad]; | 692 | [self cancelLoad]; |
| 688 | [self setNeedsRedirect:NO]; | 693 | [self setNeedsRedirect:NO]; |
| 689 | [self setRedirectCount:[self redirectCount]+1]; | 694 | [self setRedirectCount:[self redirectCount]+1]; |
| @@ -699,7 +704,8 @@ static NSLock *sessionCookiesLock = nil; | @@ -699,7 +704,8 @@ static NSLock *sessionCookiesLock = nil; | ||
| 699 | } | 704 | } |
| 700 | 705 | ||
| 701 | // See if our NSOperationQueue told us to cancel | 706 | // See if our NSOperationQueue told us to cancel |
| 702 | - if ([self isCancelled]) { | 707 | + if ([self isCancelled] || [self complete]) { |
| 708 | + [[self cancelledLock] unlock]; | ||
| 703 | break; | 709 | break; |
| 704 | } | 710 | } |
| 705 | 711 | ||
| @@ -708,10 +714,11 @@ static NSLock *sessionCookiesLock = nil; | @@ -708,10 +714,11 @@ static NSLock *sessionCookiesLock = nil; | ||
| 708 | [self setLastActivityTime:[NSDate date]]; | 714 | [self setLastActivityTime:[NSDate date]]; |
| 709 | [self setLastBytesSent:totalBytesSent]; | 715 | [self setLastBytesSent:totalBytesSent]; |
| 710 | } | 716 | } |
| 711 | - | 717 | + |
| 712 | // Find out how much data we've uploaded so far | 718 | // Find out how much data we've uploaded so far |
| 713 | - [[self cancelledLock] lock]; | ||
| 714 | [self setTotalBytesSent:[[(NSNumber *)CFReadStreamCopyProperty(readStream, kCFStreamPropertyHTTPRequestBytesWrittenCount) autorelease] unsignedLongLongValue]]; | 719 | [self setTotalBytesSent:[[(NSNumber *)CFReadStreamCopyProperty(readStream, kCFStreamPropertyHTTPRequestBytesWrittenCount) autorelease] unsignedLongLongValue]]; |
| 720 | + | ||
| 721 | + // Updating the progress indicators will attempt to aquire the lock again when needed | ||
| 715 | [[self cancelledLock] unlock]; | 722 | [[self cancelledLock] unlock]; |
| 716 | 723 | ||
| 717 | [self updateProgressIndicators]; | 724 | [self updateProgressIndicators]; |
| @@ -31,6 +31,7 @@ IMPORTANT | @@ -31,6 +31,7 @@ IMPORTANT | ||
| 31 | NSMutableArray *finishedRequests; | 31 | NSMutableArray *finishedRequests; |
| 32 | 32 | ||
| 33 | ASINetworkQueue *releaseTestQueue; | 33 | ASINetworkQueue *releaseTestQueue; |
| 34 | + ASINetworkQueue *cancelQueue; | ||
| 34 | } | 35 | } |
| 35 | 36 | ||
| 36 | - (void)testFailure; | 37 | - (void)testFailure; |
| @@ -50,8 +51,13 @@ IMPORTANT | @@ -50,8 +51,13 @@ IMPORTANT | ||
| 50 | - (void)testMultipleDownloadsThrottlingBandwidth; | 51 | - (void)testMultipleDownloadsThrottlingBandwidth; |
| 51 | - (void)testMultipleUploadsThrottlingBandwidth; | 52 | - (void)testMultipleUploadsThrottlingBandwidth; |
| 52 | 53 | ||
| 54 | +/* | ||
| 55 | +- (void)testCancelStressTest; | ||
| 56 | +*/ | ||
| 57 | + | ||
| 53 | @property (retain) NSOperationQueue *immediateCancelQueue; | 58 | @property (retain) NSOperationQueue *immediateCancelQueue; |
| 54 | @property (retain) NSMutableArray *failedRequests; | 59 | @property (retain) NSMutableArray *failedRequests; |
| 55 | @property (retain) NSMutableArray *finishedRequests; | 60 | @property (retain) NSMutableArray *finishedRequests; |
| 56 | @property (retain) ASINetworkQueue *releaseTestQueue; | 61 | @property (retain) ASINetworkQueue *releaseTestQueue; |
| 62 | +@property (retain) ASINetworkQueue *cancelQueue; | ||
| 57 | @end | 63 | @end |
| @@ -522,6 +522,28 @@ IMPORTANT | @@ -522,6 +522,28 @@ IMPORTANT | ||
| 522 | complete = YES; | 522 | complete = YES; |
| 523 | } | 523 | } |
| 524 | 524 | ||
| 525 | +// A test for a potential crasher that used to exist when requests were cancelled | ||
| 526 | +// We aren't testing a specific condition here, but rather attempting to trigger a crash | ||
| 527 | +// This test is commented out because it may generate enough load to kill a low-memory server | ||
| 528 | +// PLEASE DO NOT RUN THIS TEST ON A NON-LOCAL SERVER | ||
| 529 | +/* | ||
| 530 | +- (void)testCancelStressTest | ||
| 531 | +{ | ||
| 532 | + [self setCancelQueue:[ASINetworkQueue queue]]; | ||
| 533 | + | ||
| 534 | + // Increase the risk of this crash | ||
| 535 | + [[self cancelQueue] setMaxConcurrentOperationCount:25]; | ||
| 536 | + int i; | ||
| 537 | + for (i=0; i<100; i++) { | ||
| 538 | + ASIHTTPRequest *request = [ASIHTTPRequest requestWithURL:[NSURL URLWithString:@"http://127.0.0.1"]]; | ||
| 539 | + [[self cancelQueue] addOperation:request]; | ||
| 540 | + } | ||
| 541 | + [[self cancelQueue] go]; | ||
| 542 | + [NSThread sleepUntilDate:[NSDate dateWithTimeIntervalSinceNow:2]]; | ||
| 543 | + [[self cancelQueue] cancelAllOperations]; | ||
| 544 | + [self setCancelQueue:nil]; | ||
| 545 | +} | ||
| 546 | +*/ | ||
| 525 | 547 | ||
| 526 | // Not strictly an ASINetworkQueue test, but queue related | 548 | // Not strictly an ASINetworkQueue test, but queue related |
| 527 | // As soon as one request finishes or fails, we'll cancel the others and ensure that no requests are both finished and failed | 549 | // As soon as one request finishes or fails, we'll cancel the others and ensure that no requests are both finished and failed |
| @@ -768,4 +790,5 @@ IMPORTANT | @@ -768,4 +790,5 @@ IMPORTANT | ||
| 768 | @synthesize failedRequests; | 790 | @synthesize failedRequests; |
| 769 | @synthesize finishedRequests; | 791 | @synthesize finishedRequests; |
| 770 | @synthesize releaseTestQueue; | 792 | @synthesize releaseTestQueue; |
| 793 | +@synthesize cancelQueue; | ||
| 771 | @end | 794 | @end |
-
Please register or login to post a comment