1

The most direct and simple implementation would like this

static MySingleton *_instance = nil;

    + (MySingleton *) instance
{
    @synchronized (_instance)
    {
        if (_instance == nil)
        {
            _instance = [[MySingleton alloc] init];
        }

        return _instance;
    }
}

Actually I knew several popular posts about singleton such like Implementing a Singleton in iOS and the pop template

So my question here would be "any defects of above implementation " ?

Forrest
  • 122,703
  • 20
  • 73
  • 107
  • You should also take a look at [What does your ObjC singleton look like?](http://stackoverflow.com/questions/145154/what-does-your-objective-c-singleton-look-like) – jscs Feb 27 '12 at 02:41

2 Answers2

5

Yes, there's a big defect in your implementation. The @synchronized directive turns into a call to objc_sync_enter and a later call to objc_sync_exit. When you call the instance method the first time, _instance is nil. The objc_sync_enter function does no locking if you pass it nil, as you can see by looking at its source code.

So if two threads simultaneously call instance before _instance has been initialized, you will create two instances of MySingleton.

Furthermore, you should put the _instance variable inside the function, unless you have a reason to expose it to the whole source file.

The preferred implementation of a singleton accessor for iOS 4.0 and later uses the very efficient dispatch_once function and looks like this:

+ (MySingleton *)sharedInstance {
    static MySingleton *theInstance;
    static dispatch_once_t once;
    dispatch_once(&once, ^{
        theInstance = [[self alloc] init];
    });
    return theInstance;
}

The dispatch_once function is not available before iOS 4.0, so if you really need to support older iOS versions (which is unlikely), you have to use the less-efficient @synchronized. Since you can't synchronize on nil, you synchronize on the class object:

+ (MySingleton *)sharedInstance {
    static volatile MySingleton *theInstance;
    if (!theInstance) {
        @synchronized (self) {
            if (!theInstance)
                theInstance = [[self alloc] init];
        }
    }
    return theInstance;
}
rob mayoff
  • 375,296
  • 67
  • 796
  • 848
1

What should my Objective-C singleton look like? is a good discussion for singletons (as Josh pointed out), but to answer your question:

No, that will not work. Why?

@synchronized needs an allocated and constant object to synchronize against. Think of it as a point of reference. You are using _instance which will initially be nil (not good). A nice part of objective-c is that classes are themselves objects so you can do:

@synchronized(self)

Now as far as defects, once you synchronize against the constant object that is the class itself (using self) you will have an defect-free singleton, however you will take on the cost of synchronization overhead every time you access your singleton which can have significant performance ramifications.

The easy mechanism for alleviating this is identifying that the creation need only occur once, and after that all subsequent calls will be returning the same reference which will never change over the course of your processes lifetime.

Identifying this we can wrap your @synchronization block in a nil check to avoid that performance hit after the singleton has been created:

static MySingleton *_instance = nil;

+ (MySingleton *) instance
{
    if (!_instance)
    {
        @synchronized (self)
        {
            if (!_instance)
            {
                _instance = [[MySingleton alloc] init];
            }
        }
    }
    return _instance;
}

Now you have a double-NULL-check implementation for you singleton. But wait! There's more to consider! What? What could be left? Well for this example code, nothing, but in the case where there is work to be performed on the singleton as it is being created, we need to make some considerations...

Let's look inside the second nil check and add some additional code for a common singleton scenario where additional work is need.

if (!_instance)
{
    _instance = [[MySingleton alloc] init];
    [_instance performAdditionalPrepWork];
}

This all look great, however there exists a race condition between the assignment of the alloc-inited class to the _instance reference and the point where we perform the prep work. It is feasible for a second thread to see that _instance exists and use if BEFORE the prep work has completed creating a race condition with undefined (and potentially crashing) result.

So what do we need to do? Well, we need to have the singleton completely prepped before being assigned to the _instance reference.

if (!_instance)
{
    MySingleton* tmp = [[MySingleton alloc] init];
    [tmp performAdditionalPrepWork];
    _instance = tmp;
}

Easy, right? We've solved the problem by delaying the assignment of the singleton to the _instance reference until after we've prepped the object completely, right? In a perfect world, yes, but we don't live in a perfect world and as it turns out we need to account for an outside force mucking with our perfect code...the compiler.

Compilers are very advanced and work hard to improve efficiency of code by eliminating seeming redundancies. Redundancies like using a temporary pointer that could seemingly be avoided by directly using the _instance reference. Curses efficient compilers!

It's okay though, there is a low level API provided by every platform that has this problem. We'll use a memory barrier between the prepping of the tmp variable and the assignment of the _instance reference, aka OSMemoryBarrier() for iOS and Mac OS X platforms. It simply acts as an indicator to the compiler that the code before the barrier should be taken into consideration independently of the code that follows, thus eliminating the compiler's interpretation of code redundancy.

Here's our new code in the nil check:

if (!_instance)
{
    MySingleton* tmp = [[MySingleton alloc] init];
    [tmp performAdditionalPrepWork];
    OSMemoryBarrier();
    _instance = tmp;
}

By George, I think we've got it! A double-NULL-check singleton accessor that is 100% thread safe. Is it overkill? Depends on if performance and thread safety are important to you. Here's the final singleton implementation:

#include <libker/OSAtomic.h>

static MySingleton *_instance = nil;

+ (MySingleton *) instance
{
    if (!_instance)
    {
        @synchronized (self)
        {
            if (!_instance)
            {
                MySingleton* tmp = [[MySingleton alloc] init];
                [tmp performAdditionalPrepWork];
                OSMemoryBarrier();
                _instance = tmp;
            }
        }
    }
    return _instance;
}

Now if you don't have the prep work to do in objective-C, simply assigning the alloc-inited MySingleton is just fine. In C++, however, order of operations dictates it necessary to do the temp-variable-with-memory-barrier trick. Why? Because the allocation of the object and assignment to the reference will occur BEFORE the construction of the object.

_instance = new MyCPPSingleton(someInitParam);

is (effectively) the same as

_instance = (MyCCPSingleton*)malloc(sizeof(MyCPPSingleton));  // allocating the memory
_instance->MyCPPSingleton(someInitParam);                     // calling the constructor

So if you never use C++, nevermind, but if you do - be sure to keep that in mind if you plan on applying a double-NULL-check singleton in C++.

NSProgrammer
  • 2,387
  • 1
  • 24
  • 27