Why is [SVHTTPRequest start] being executed on the main thread?

[tciuro]

Hi Sam, this is not an issue but a question.

In [SVHTTPRequest start] you have written:

- (void)start {

    if(self.isCancelled) {
        [self finish];
        return;
    }

    if(![NSThread isMainThread]) { // NSOperationQueue calls start from a bg thread (through GCD), but NSURLConnection already does that by itself
        [self performSelectorOnMainThread:@selector(start) withObject:nil waitUntilDone:NO];
        return;
    }

    ...
}

Question: isn't executing the SVHTTPRequest on the main thread defeating the purpose of NSOperationQueue's concurrency model? I can see that one would have to do that in order to call the completion/failure block on the main thread. But then, unless I'm mistaken, it seems to me that launching several SVHTTPRequest would all end up on the main thread, right? Wouldn't these operations saturate the same core?

Thank you,

-- Tito

[samvermette]

@tciuro as I say in the comment, I make this check because by default NSOperationQueue calls start from a bg thread (which causes the delegate methods to not always get called, among other issues that I can't remember). But when you fire an NSURLConnection instance, it already fires off a new thread (that's the important one; preparing the NSOperation doesn't take much time, but NSURLConnection waiting for a response might take a while so we need it to happen in the background).

We don't need both NSOperationQueue and NSURLConnection to run on background threads, only NSURLConnection. Does that make sense?

[samvermette]

@tciuro it would probably make more sense if I had described all the issues happening when not doing that check. It took be a solid hour to figure out everything weird that was happening, and eventually adding this check solved it all. Sorry I can't be more specific about what kind of weird stuff was going on (all I can remember is methods not getting called).

[tciuro]

@samvermette as I suspected, the current way it's suboptimal. Scheduling every single operation on the main thread/queue will potentially saturate the main runloop, delaying the execution of high-priority events such as touch events. Simply put, it won't scale for apps sending lots of requests or requests retrieving large amounts of data.

The fact that an async NSURLConnection already fires a thread is orthogonal. The point is that the delegate methods will be called on the main queue, and that could potentially hurt the user experience. The solution is rather simple really: just invoke the relevant block on the main thread once it has finished. Something along these lines:

dispatch_async( dispatch_get_main_queue(), ^{
    if (success) {
        successHandlerBlock(data, error);
    } else {
        failureHandlerBlock(nil, error);
    }
});

Scheduling the NSOperation in a queue other than the main queue is the way to go.

[samvermette]

@tciuro I tried what you said and I'm still having issues. As expected, removing

if(![NSThread isMainThread]) { // NSOperationQueue calls start from a bg thread (through GCD), but NSURLConnection already does that by itself
    [self performSelectorOnMainThread:@selector(start) withObject:nil waitUntilDone:NO];
    return;
}

makes the demo project crash when tapping "github get request". I tried making it call the completion blocks on the main thread like you suggested:

dispatch_async(dispatch_get_main_queue(), ^{
    if(self.operationCompletionBlock && !self.isCancelled)
        self.operationCompletionBlock(response, self.operationURLResponse, error);

    [self finish];
});

But I keep getting the same crash:

** Terminating app due to uncaught exception 'NSUnknownKeyException', reason: '[<NSConcreteData 0x6867450> valueForUndefinedKey:]: this class is not key value coding-compliant for the key repository.'

Basically it looks like the response is not playing nice with the multi-threading...

[samvermette]

@tciuro whoops, that crash was because the demo project used a deprecated version of the GitHub API.

I tried it again with a request URL that actually returns something and the problem is as I recalled it when I added that isMainThread check: the delegate calls are never called for some of the requests.

Now that I moved the json parsing and file writing to a bg queue I had to add to force the completion block to be called on the main thread, so if you feel like trying it all you need to do now is remove the isMainThread check.

[samvermette]

Fixed in 276ae99.