Thursday, April 07, 2011

Singletons: You're doing them wrong

This post feels a bit odd, as generally I fully agree with Dave Dribin on how wrong singletons are and that you should avoid them if possible. However on Stack Overflow i've seen far too many people write bad code that is horrible for several reasons.

Basic Example

Singletons are fairly easy to create. Here is a basic example...

+(MyClass *)singleton {
 static MyClass *shared = nil;
 
 if(shared == nil) {
  shared = [[MyClass alloc] init];
 }
 return shared;
}

However this is wrong on several levels. Firstly, this isn't thread safe, so what happens if multiple threads all try to access this at the same time? There is no reason 1 thread couldn't be in the middle of allocating the object while the other one is trying to access the object. This is actually what Apple shows in its documentation.

If you must use singletons, use dispatch_once()

dispatch_once() solves the problem of safely being able to create a singleton in that (1) it guarantees that the code in the block will only be called once for the lifetime of the application (2) its thread safe as I noted in a previous article and (3) its faster than other methods like using @synchronize(),etc...

"If called simultaneously from multiple threads, this function waits synchronously until the block has completed."

So you should be writing it like this...

+(MyClass *)singleton {
 static dispatch_once_t pred;
 static MyClass *shared = nil;
 
 dispatch_once(&pred, ^{
  shared = [[MyClass alloc] init];
 });
 return shared;
}

This is a safe way to create a singleton. There is no way to accidentally create it twice, its fast and it is thread safe.

Conclusion

Avoid singletons if at all possible because it is easy to abuse them. Unlike Dave Dribin I don't think they are evil, but I really try and avoid them when possible. If you must create them, do it right and don't create them like the first example, use dispatch_once(). It is less code and the correct way to do it.

29 comments:

יניב (hAcx) said...

You should point out that this only works on 4.0 and up.
Blocks were introduced in 4.0.

Unknown said...

For OS's prior to 10.5 or 4.0, you can use this hybrid code which drops down to OSAtomicCompareAndSwapPtrBarrier() where GCD isn't available.

There's a discussion of the OSAtomic routines and the entire problem field on my blog, although the new theme appears to have done something horrible to the inline code samples. Sorry about that.

Jano said...
This comment has been removed by the author.
Dan said...

That's an interesting way to do it. I wouldn't have thought of it.

For what it's worth, one case where singletons are safe is when the singleton itself has no fields. Such a singleton is an object that represents pure behavior. There would be no advantage to creating a second instance, since it would be indistinguishable from the original instance (except for its address, of course).

ferbass said...

That's a good point man, people like use singletons but don't understand the risks and a dirt code

gidaeyeo ([email protected]) said...

Good posting!
I used @sychronized. From now, I'll use dispatch_oncd as you posted.
I wrote a post about this on my korean blog :)
Thanks a lot~

Dan said...

@Daniel - why create a singleton without any state? Why not just use static methods in that case?

Dan said...

You're right, it seems weird to create ANY object without state, singleton-ness notwithstanding. The short answer is that you do so if you want to participate in polymorphic dispatch. Suppose you are constructing some other object or calling some function, and that constructor or function requires a delegate object. Sometimes, you will pass in a delegate object that has fields of its own and complex behavior. However, sometimes the callback behavior you want is so simple that it isn't parameterized at all. The only parameterization you need is to pick the class of the delegate object. Such a delegate object wouldn't need any state, so the class wouldn't have any fields. At that point, it's safe to make the class a singleton, since every instance is equivalent. There's no advantage to constructing more than one.

The case that comes to mind to me is, if you were building some kind of calculating application, and you wanted the operation to be represented by an object. You might have an AddOperation, a MultiplyOperation, and so on. Depending on how you structure your code, these operations might not need to store any data - any parameterization is wrapped up in the class itself. There wouldn't be any reason to create more than one instance of AddOperation, so make it a singleton to communicate to the users that all the instances are the same.

Anonymous said...

Singletons are not evil if they actually express the fact that there can only be one instance of an object, or that there is a special instance of an object that is unique across the entire program.

Apple's own libraries use singletons. [NSFileManager defaultFileManager] anyone?

Colin Wheeler said...

@Anonymous

I've never thought Singletons are evil, some other may disagree with me on that.

The reason singletons are controversial is because they are akin to global variables and with multithreading accessing/setting their state safely from multiple threads becomes something really easy to do wrong if your not careful. This is one reason why apple made atomic a default property on everything unless you explicitly set nonatomic.

MarkDalrymple said...

Singletons also make things harder to test.

@anonymous, Apple lets you alloc/init your own file managers. Since NSFileManager can have a delegate, it can make sense to make your own and hang on to them, so NSFileManager is definitely not a singleton situation where every object is the One True Object.

I've helped enough new programmers unsnarl "Well, I though this *was* going to be a singleton, but it ended up not" that I feel singletons are an advanced pattern, not to be taken lightly.

LJ Gould said...

I use this technique all the time, but lesson learned: dispatch_once will deadlock if the something in the -init method calls the +singleton method.

Not that I should have been doing that anyway...

Unknown said...

Could you comment on this alternative:

+(MyClass *)singleton {
static MyClass *shared = [[MyClass alloc] init];

return shared;
}

TIA,
Léo

Colin Wheeler said...

@leo
its not thread safe. the variable is in static storage, but i'd be weary of using that in a multithreaded app.

Jamie said...

I think this example is wrong, this code creates a new singleton every time it's called and leaks the previous value of shared. You're supposed to assign shared to nil in the file/compilation unit's scope, or in +initialize, not on every call to singleton.

Dan said...

@Jamie

You should read up on static local variables. The static qualifier makes shared into a single global variable, but which is only accessible from this function. Static local variables are part of the C lineage of Objective-C.

Karl said...

Here's a macro that does singletons properly for all iOS versions:

https://github.com/cjhanson/Objective-C-Optimized-Singleton

Anonymous said...

Leaving aside the pros and cons of singleton design generally, if you want a self-contained singleton behaviour for a class, consider implementing the built-in + (void)initialize method to create and store the instance - this method is invoked only once when a class is loaded and is thread safe. Blocks are iOS4 onwards only, so they break backwards compatability.

Colin Wheeler said...

@Anonymous if your still working on iOS 3 you should be getting off of it fast. At this point I don't care at all about breaking iOS 3 compatibility. If I could I would be making iOS 5 only apps, but some people are still making the transition to it so provide iOS 4 compatibility for a while.

Also you didn't read the post as dispatch_once() IS thread safe and explicitly mentioned as being so in the documentation and why I am using it in the code sample provided in the article.

Musesum said...

I replaced my singletons and get a semaphore_wait_trap

searching on semaphore_wait_trap and dispatch_once_t https://discussions.apple.com/thread/3399246?start=0&tstart=0

since the code was working, am reverting back.

Jens Ayton said...

Colin, you didn’t answer the latest Anonymous’s point about +initialize, the idiomatic Objective-C way of handling this.

As for the various giant hacks to provide fallback behaviour for dispatch_once(), don’t go near them. If you need that sort of backwards compatibility, use pthread_once().

Davide said...

@Musesum I have the same problem, dispatch_once_t waits in deadlock on semaphore_wait_trap ...

Joker said...

One more way to create thread safe singletone is:

@implementation Singleton
static Singleton *_sharedInstance = nil;

+ (Singleton *)sharedInstance {
@synchronized(self) {
if (!_sharedInstance) {
_sharedInstance = [[Singleton alloc] init];
}
}
return _sharedInstance;
}

@end

Anonymous said...

How would you prevent other client code from calling [[MyClass alloc] init] to create a different instance other than the shared one under ARC?

Anonymous said...

@Joker why @synchronized(self) in a static method? There isn't self in a static method, the receiver isn't an object.

Anonymous said...

@Joker in a static method the receiver isn't an object, so you can't use self.

Anonymous said...

I've started initializing all my class singletons in the +initialize method.

I found that iOS 6 provides a dispatch_once for +initialize by setting a breakpoint and examining the stack. iOS 5 appears to provide a more conventional lock of some kind before calling.

It nicely simplifies the accessors' code, and you don't need to allocate an extra static for each singleton if you can clump them all +initialize.

Unknown said...

Good topic, it helped me to sort out the crash issue that the old method singleton cause. Simple enough but strong!

Anonymous said...

@Ramy AL ZUHOURI You can use "self" in static methods. It returns the class.

 
...