Slow cancelAllOperations ( with workaround/fix ) #158

Open
stefanop opened this Issue Mar 24, 2011 · 1 comment

Projects

None yet

2 participants

@stefanop

I'm using a ASINetworkQueue to download several hundreds of files, using a couple of parallel threads.
If user cancel the transfer, I call cancellAllOperations.

This call takes a very long time on the real device.

( I've tryed both an old version and latest git tree, the behaviour is the same )

I could workaround / solve this problem with two lines of code in ASIHTTPRequest cancel.

here is the patch:

----------- CUT HERE ----
diff --git a/ASI_HTTP_Request/ASIHTTPRequest.m b/ASI_HTTP_Request/ASIHTTPRequest.m
index deff3a8..b056a5f 100644
--- a/ASI_HTTP_Request/ASIHTTPRequest.m
+++ b/ASI_HTTP_Request/ASIHTTPRequest.m
@@ -699,7 +699,9 @@ static NSOperationQueue *sharedQueue = nil;

  • (void)cancel
    {
    • [self performSelector:@selector(cancelOnRequestThread) onThread:[[self class] threadForRequest:self] withObject:nil waitUntilDone:NO];
    • if (!cancelled) {
    •    [self performSelector:@selector(cancelOnRequestThread) onThread:[[self class] threadForRequest:self] withObject:nil waitUntilDone:NO];  
      
    • }
      }
  • (void)clearDelegatesAndCancel
    ----------- CUT HERE ----

I've just avoided to call cancelOnRequestThread selector if current request is already cancelled.
Accessing directlry cancelled member should not pose a threading problem, the worst thing that can happen is an extra call to cancelOnRequestThread ( that would return imediately ).

With this change in place, cancelAllOperations is almost instantaneous, without it, it can take 40/50 seconds on the real device.

@akisute
akisute commented Sep 29, 2011

I've met the same problem in my project, which requires 2000+ file downloads at a time (insane? yep). I tried to workaround this by not editing the ASIHTTPRequest itself but by subclassing it and override the cancel method like this:

// in subclasses...
- (void)cancel
{
    // Quite dirty way to get a cancel lock
    NSRecursiveLock *lock = [self valueForKey:@"cancelledLock"];
    NSAssert(lock, @"lock must be aquired");
    [lock lock];
    BOOL isExecuting = self.isExecuting;
    if (!isExecuting) {
        // Cancel the request without using the requested thread.
        // since the request is not running, we don't have to use the main thread, which could be blocked by thousands of request objects
        [self performSelector:@selector(cancelOnRequestThread)];
        [lock unlock];
        return;
    }
    // Cancel ordinarily because the request is still alive
    [lock unlock];
    [super cancel];
}

The result wasn't as good as the stefanop says. I haven't tried his way yet but if it works we might want to apply his patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment