Aral Balkan

I just encountered a hairy yet rather funny bug in Feathers that I thought I'd share with you. It was one of those that leaves you scratching your head even though it's just staring you in the face. (Granted, when you have a large code-base and multiple threads, locating the area of the code where the issue is staring you in the face can be a chore in and of itself.) So, without further ado…

What's wrong with his code?

NSHTTPURLResponse *urlResponse;
NSError *error;
NSData *resultData = [NSURLConnection sendSynchronousRequest:req returningResponse:&urlResponse error:&error];

NSInteger errorCode = [error code];

// Do some error handling...
}

(Said code appears inside an NSOperation instance and hence the use of the synchronous request.)

At first glance, it might appear like there isn't a problem with it but when you run it, you might (it's random, depending on the state of your app's memory at that time) get the dreaded EXC_BAD_ACCESS (if running on the device) or, if you're running in the simulator, the more helpful:

*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[NSCFString code]: unrecognized selector sent to instance 0x4b9994'

(To track down the method call that caused the EXC_BAD_ACCESS, type display /s $r1 in the gdb debugger console and you'll see that the method "code" caused it. The simulator error is still more helpful though.)

Wait a second… the only thing we're passing the code message to here is the NSError instance. But the error is telling me that I'm trying to send the selector to a string instance. That's not right. So I logged error, and lo and behold, I got a string ("isExecuting").

Do you see the problem yet?

Look at where I'm declaring the urlResponse and error variables. I forgot to set them to nil. If you declare a pointer, remember that it simply points to some address in memory (and to whatever garbage or otherwise there may be at that address). In this case, the garbage it was pointing to just happened to be a string and this was what caused the crash.

So the code should have read:

NSHTTPURLResponse *urlResponse = nil;
NSError *error = nil;
NSData *resultData = [NSURLConnection sendSynchronousRequest:req returningResponse:&urlResponse error:&error];
// ...

Remember folks, nil is your friend.

Update: As pointed out in the comments, you should really be checking the return value of the call (if (!resultData) {). That would have avoided the bug also.

Comments

• You should update your post to reflect that the value of the error variable is undefined unless the method accepting it by-ref returns a failure condition. On synchronous networking, you've taken an asynchronous API and performed it synchronously, in a concurrent context. This is wasteful, and, beyond the issues already mentioned, it removes the worker thread from the thread pool until the I/O is complete. If you've blocked all the threads in the pool, the kernel is forced to allocate further threads to handle further operations. Schedule your asynchronous APIs in a run loop that is serviced, the main thread is fine for this. Or if you must, create a single background I/O thread that services a run loop, though there's rarely any need to do this.

  by Keith on 2010-10-13 18:17:22

• Hi Klaas, You're right, checking for the for return value first would have saved me the heartache too. (Some bits of my code were written in my early days with Objective-C.) On not doing synchronous calls in NSOperations, though, I have to disagree. Do you have an official source for that? NSOperations are perfect for doing synchronous calls, etc. without blocking the main thread and they've been working splendidly for me so far.

  by Aral on 2010-10-13 14:32:30

• Thanks for updating the post, though now, there's nothing actually wrong with your code sample. If `resultData == nil`, then `error` _will_ point to a valid error. There's no need to initialise it to nil. Checking the resultData ensures you're only ever reading a valid error. The problem with performing I/O synchronously is conflating synchronicity with concurrency. Here, you have no asynchronicity, you only appear to because you've used concurrency. The performance issue comes from that if you've blocked all the thread pool threads, and continue to schedule work (bear in mind that the system is free to schedule work on them too) the kernel will be forced to allocate more threads to the thread pool to handle the 'load'. This takes time, memory and introduces latency between the tasks/operations being scheduled and actually being performed. Though the performance issue this causes may not be made immediately obvious to your application. For every thread you create, the kernel needs to allocate, track, and schedule it. If you were to multiply this type of behaviour across multiple processes you'll end up with a significantly larger number of threads parked on I/O than there needs to be. Amortising the cost of your I/O is part of being a good citizen. As a general rule, all I/O should be asynchronous.

  by Keith on 2010-10-14 09:21:28

• Hey Keith, Sorry, I shouldn't attempt to multitask – reverted the code to illustrate the original point and added a note at the end.

  by Aral on 2010-10-18 16:39:18

• In the first example you should be checking wether resultData is nil. Only if that is true can you guarantee that the error object is valid. See: http://developer.apple.com/library/ios/#documentation/cocoa/Conceptual/ErrorHandlingCocoa/CreateCustomizeNSError/CreateCustomizeNSError.html%23//apple_ref/doc/uid/TP40001806-CH204-SW1 Also it's considered bad practice to run synchronous requests in an NSOperation. It avoids Apple's optimized threading code and I believe synchronous requests don't properly handle redirects and authentication. - Klaas Pieter

  by Klaas Pieter on 2010-10-13 13:50:43

• Seriously? What an amateur mistake. Can't believe you're still writing code like that. Unsubscribed.

  by Bob on 2010-10-13 14:03:03

• ROTFL. Sorry to lose you, Bob aka Mr. Perfect :)

  by Aral on 2010-10-13 14:26:35

• All I can find right now is this: http://www.cocoabuilder.com/archive/cocoa/281178-running-nsurlconnection-from-within-an-nsoperation.html

  by Klaas Pieter on 2010-10-13 14:53:46

• Nil/ null, 0 & false are truly the friends of a diligent programmer. Whilst some languages, eg Java and AS3, auto-initialize variables, other languages do not. Objective-C appears to be one such language. A good developer will work with a number of languages during their career and so always explicitly initializing variables is a good habit to get into. Of course this is easier said than done. I've only been working with AS3 for around three years, yet already I'm questioning why I explicitly initialize my variables to their default values and have started not bothering...

  by David Arno on 2010-10-13 14:45:03

• Klaas, Keith, I updated the post to check the return value. Thanks!

  by Aral on 2010-10-13 21:32:39

• Keith, If I'm not seeing any performance degradation from running a synchronous network call asynchronously via an NSOperation, is it not a premature optimization to complicate the code by running it asynchronously?

  by Aral on 2010-10-13 21:39:40