Next article: Deconstructing Apple's Copyright and Trademark Guidelines
Previous article: Friday Q&A 2009-11-06: Linking and Install Names
Tags: cocoa dangerous threading
It's another Friday, and so time for another Friday Q&A. This week, Quentin Carnicelli (one of the guys who signs my paychecks) suggested that I talk about dangerous API calls in Cocoa.
First, let me clarify what I mean by a dangerous call. I don't mean something that is obviously dangerous, like -[NSFileManager removeFileAtPath:handler:]. That call is dangerous because you could use it to wipe the user's entire hard drive, but it's not interesting, because it's obvious that it can do this. Instead, I'm going to cover calls which are subtly dangerous, things which you might easily use in an innocent manner, only to discover later that bad things can occur.
Without further ado, here are my dangerous calls:
-[NSTask launch]
This call is dangerous because it throws exceptions when it fails, and it can fail for completely mundane problems, like trying to execute a file that doesn't have the executable bit set. Because the Cocoa tradition is to only throw exceptions for programmer errors, this can cause trouble for the unwary programmer who doesn't wrap this in a @try block.
-[NSTask waitUntilExit]
This one looks tremendously innocent. The problem is that, if the task is still running, it will run the runloop to wait for the task to exit. Even that would not be a problem, except that it runs the runloop in the default mode. Because of this, all timers, callbacks, etc., that are installed in the default mode will continue to run, but will be running in a context they did not expect. This call looks like a simple blocking call, but it can potentially lead to the invocation of random timers or other callbacks. This in turn can easily lead to deadlocks or data corruption.
+[NSTimer scheduledTimerWithTimeInterval:target:selector:userInfo:repeats:]
This returns a timer whose memory management is difficult to get right. Many people simply stash the return value in an instance variable. This is legitimate, because the runloop is documented to retain the timer. However, the runloop will release it as soon as it's invalidated, which will happen once a non-repeating timer fires, and also upon explicit invalidation. It's easy to get this wrong and end up with a dangling reference, and a lot of code has this problem. To make things even more fun, the timer retains its target, so if you retain the timer, you're likely to set up a retain cycle.
NSHost
Yes, this entire class is dangerous, and should not be used. Why? It's an unfortunate confluence of two otherwise unrelated properties of the class.
The first property is that NSHost has a blocking API. It accesses network resources, and as such any call to it can take an indefinite amount of time. This is fine by itself.
The second property is that NSHost returns shared objects, but NSHost is not thread safe. This means that it can only be safely used on the main thread.
Put the two together, and you have a class which can block any time you use it, but which can only be used on the main thread. Since it's unacceptable to indefinitely block the main thread of an application, this basically means that you can't use NSHost.
NSBundle
This one has half of the problems of NSHost. NSBundle returns shared objects, but is not thread safe, so it's main-thread only. It's still safe to use from the main thread. The reason I mark it as dangerous is because the fact that it's unsafe to use from secondary threads is not really documented, but rather has to be inferred from the fact that it's not thread safe and the fact that the instances are shared, and it can be tempting to use it from other threads.
-[NSBundle unload]
Because it's very easy to make references to memory within a bundle's code (e.g. by creating instances of an Objective-C class defined within the bundle, or by referring to literal strings defined within the bundle), and extremely difficult to make sure that all such references are gone, it's generally not safe to unload code on Mac OS X. If any such dangling references remain, your application will crash.
-[NSImage imageNamed:], followed by mutation
This call is very convenient, but it returns a shared object. A lot of code will make this call, and then immediately start resizing or drawing into the object that it returns. This can mess up the shared image for any other code that uses it. If you're going to modify the image, then you need to copy it first, but this isn't immediately obvious.
-[NSDate timeIntervalSinceReferenceDate] used for elapsed time
How many of you have written code like this?
BOOL success;
NSTimeInterval start = [NSDate timeIntervalSinceReferenceDate];
do
success = [self trySomething];
while(!success && [NSDate timeIntervalSinceReferenceDate] - start <= kTimeoutInterval);
So what's the problem? -timeIntervalSinceReferenceDate uses the system clock, which can be changed! Imagine that your program enters this loop, but before it can exit it, the system clock is set back by a day. Your timeout has now gone up to a day! Or imagine the opposite, that the system clock is suddenly set forward. Your timeout will suddenly become zero.
Mac OS X tries to make time adjustments smooth. For example, if the NTP client discovers that your clock is a few seconds out of whack, it will gradually pull it back in rather than making the adjustment all at once. However, sudden adjustments are still possible, even if rare, and your code should be robust against them. Even the gradual adjustments will cause your timeouts to change duration, albeit not by a huge amount.
The bad news is that Cocoa doesn't make it easy to do this right. You want to use a consistent, non-adjustable for elapsed time, but Cocoa has no APIs for such a thing. The OS does, so you just have to drop down a level.
My preferred API for this task is mach_absolute_time. However, this returns a value that's in terms of arbitrary timebase units, not something comfortable like seconds or nanoseconds. It's not hard to convert, but requires some work. Apple has two examples of how to convert the return value into something sensible.
Two other reasonable alternatives are the Carbon calls Microseconds and UpTime.
Distributed Objects
I've said it before, but Distributed Objects need special care and attention. The trouble is that DO looks like a completely transparent way to access objects in other processes, but it doesn't entirely succeed at this. It can still be a good way to do IPC, but it requires more care than you might think from reading through the documentation on it. For details on why that's so, my original post on the subject covers it all.
Conclusion
No, that's not a dangerous Cocoa call, it just means that we're done for the week. Come back in seven days for another exciting edition. In the meanwhile, keep those topic suggestions flowing. Friday Q&A is driven by your ideas, so if you'd like to see a topic covered here, let me know.
Comments:
That's the first API that comes to mind immediately. But I've been hit by exceptions before. Programmer error can be a very ambiguous term for example the NSTask execute flag error could be called programmer error with a bit of a stretch…
At first when I saw your tidbit about NSDate, I thought "Too bad we can't use NSDate for this, it's nice to use something high-level when we can" but of course I didn't stop there. You've inspired me to write and add this to my library of OSS:
#import <mach/mach_time.h>
@interface SDTiming : NSObject {
uint64_t start;
}
- (void) mark;
- (uint64_t) elapsed;
@end
@implementation SDTiming
- (void) mark {
start = mach_absolute_time();
}
- (uint64_t) elapsed {
uint64_t end = mach_absolute_time();
uint64_t elapsed = end - start;
Nanoseconds elapsed_nano = AbsoluteToNanoseconds(*(AbsoluteTime*) &elapsed);
return *(uint64_t*) &elapsed_nano;
}
@end
P.S. I really hope the
NSHost is indeed the devil. Speak not its name.
This means any code working with potentially-remote objects needs to be exception-safe. But you can pass remote objects to Cocoa APIs, which can be hit by those exceptions internally when calling the object ... bad news.
(An easy way to get that problem is to pass a URL as a parameter to a DO method. NSURL is for some reason proxied instead of copied, so any usage of that URL on the other side results in a DO call and potentially an exception.)
Steven Degutis: I've now made the code tag public, so that'll fix code for the future. Example:
while(1)
destroy();
Adam Maxwell: Note that CFBundle is safe, although it's difficult to tweeze this out of the documentation. As I recall, it's not on the master list of thread safe CF classes, but there's a section buried somewhere in the CF threading docs that says that any CF API which vends shared instances will make sure that they're safe to use from multiple threads.
As for NSPipe and NSFileHandle, I've seen NSFileHandle throw exceptions for any error returned by the POSIX layer, including EINTER, which is crazy, and it was my intent to include it in my list here. However, I couldn't reproduce that no matter how hard I tried. I speculate that this may be fixed on 10.6.
It'd be nice if NSFileHandle didn't throw exceptions for errors, although it's still documented to do that in 10.6. The -[NSTask launch] behavior you mention is worse, though.
EventTypeSpec eventType;
eventType.eventClass = kEventClassSystem;
eventType.eventKind = kEventSystemTimeDateChanged;
InstallApplicationEventHandler(...)
or on 10.6 using NSSystemClockDidChangeNotification.
[NSDate date] has the same problem as timeIntervalSinceReferenceDate. That problem didn't occur to me—thanks for this post.Jens Ayton: Good catch, I didn't notice that one. Of course, NSProcessInfo is, guess what, not thread safe, and is a shared object, so that's only useful if you need that information on the main thread.
But then what about +setHostCacheEnabled and +flushHostCache? It seems like this would disable the shared object behaviour that makes it unsafe to use from different threads.
Also, there is this comment in NSHost.h (10.4 through 10.6 SDKs):
// NSHost does not implement any caching any longer
So now I'm very confused! :)
originalgeek: That's an incredibly short-sighted view. In the best case, bugs would get fixed for 10.7, which you could reasonably start requiring in only another 2-3 years, depending on when it ships and your own personal taste. Very few people are still coding to Panther. My employer recently moved everything to requiring Leopard, and I haven't written Panther-capable code in several years. Even if you do require Panther yourself, it's still a very short-sighted view: would you like it if Panther still had all the bugs that 10.0 did? Progress benefits you, even if you're slow to follow it.
I'm not real big on filing bugs myself, because Apple's process is fairly broken. But the idea that it's pointless because it'll take too long for you to take advantage of a fix is just not a good reason.
That way you don't need to mess with Mach timers directly.
notify_register_dispatch(kNotifyTimeZoneChange, ¬ify_token, dispatch_get_global_queue(0, 0), ^{});
...however I was actually trying to point at dispatch_time (not dispatch_walltime) as being suitable for timeouts in a way timeIntervalSinceReferenceDate isn't.
I didn't have a compare in mind:
BOOL success;
__block volatile BOOL give_up = NO;
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kTimeoutInterval), dispatch_get_global_queue(0, 0), ^{ give_up = YES;});
do
success = [self trySomething];
while(!success && !give_up);
That would be better as a function that takes a block and timeout (unless you want uniform timeouts) and runs the block until it returns YES or until the timeout expires...
You could "even out the odds" by not doing dispatch_after until after the first try (if and only if it fails), but then the code is getting kind of large...
So yes, it isn't appropriate to use in (most) places absolute performance is required. I _would_ use it to get a block interface like:
runWithTimeout(kTimeoutInterval, block)
since that sort of clarity is worth the modest performance cost...then again that doesn't require use of dispatch_time, you can use straight mach time calls and still pass in a block to execute (and blocks are performance competitive with function pointers as long as you don't need to Block_copy, which this function wouldn't need to).
[1] http://lists.cs.uiuc.edu/pipermail/cfe-dev/2009-January/004001.html
[2] http://clang.llvm.org/get_started.html
// NSHost does not implement any caching any longer
waitUntilExit!
What's the alternative? Should I run it on main thread (and hope for nested runloops or something?)
Can I make it a just-blocking-call?
waitUntilExit is typically safe on secondary threads. It would be dangerous only if you're actively running a runloop on that secondary thread and are scheduling things in NSDefaultRunLoopMode that shouldn't be running during the call to waitUntilExit. Otherwise, it will not be a problem. In fact, one way to make this call safe would be to proxy it over to another thread where you know that these things aren't being done, and wait for it to complete there before proceeding.NSPipe documentation explicitly states that you don't need to do anything special to close it out ("You don’t need to send closeFile to this object or explicitly release the object after you have finished using it.").
However, if you're using a lot of pipes, and garbage collection doesn't kick in early enough you will get an exception when you run out of file handles. Closing them with closeFile is therefore something that I've learned the hard way to always do.
Question... if +[NSTimer scheduledTimerWithTimeInterval:target:selector:userInfo:repeats:]
is a dangerous call. What would you suggest using instead?
NS* == Not Safe
Another NSTask method that can unexpectedly throw an exception is terminate. I think it happens if the launch initially succeeds, but then fails for some later reason, you then call terminate and it throws a "not launched" exception.
The new link is at:
https://developer.apple.com/library/mac/qa/qa1398/_index.html
Comments RSS feed for this page
Add your thoughts, post a comment:
Spam and off-topic posts will be deleted without notice. Culprits may be publicly humiliated at my sole discretion.
There a lot of classes that will throw exceptions when you least expect. The "only throw exceptions for programmer errors" rule is stretched too thin I think.
Again, great post.