Is the Singleton Class NetworkManager in Apple's Sample MVCNetworking correct?

Question

Here is the link to the sample code http://developer.apple.com/library/ios/#samplecode/MVCNetworking/Introduction/Intro.html

Below is the code snippet from the file NetworkManager.m

+ (NetworkManager *)sharedManager
// See comment in header.
{
    static NetworkManager * sNetworkManager;

    // This can be called on any thread, so we synchronise.  We only do this in 
    // the sNetworkManager case because, once sNetworkManager goes non-nil, it can 
    // never go nil again.

    if (sNetworkManager == nil) {
        @synchronized (self) {
            sNetworkManager = [[NetworkManager alloc] init];
            assert(sNetworkManager != nil);
        }
    }
    return sNetworkManager;
}

Obviously there are thread safe issues here. Two NetworkManager instance may be created when there are more than one thread. So Apple made a mistake, right?

Answer 1

Yes, you are right. It will have problem in concurrency environment. A better way is using double check before alloc:

+ (NetworkManager *)sharedManager
{
    static NetworkManager * sNetworkManager;
    if (sNetworkManager == nil) {
        @synchronized (self) {
            if (sNetworkManager == nil) {
                sNetworkManager = [[NetworkManager alloc] init];
                assert(sNetworkManager != nil);
            }
        }
    }
    return sNetworkManager;
}

And there are lots of way to write singleton using Ojbective-C, check this post: What should my Objective-C singleton look like?

Update

BobCromwell is right. The double check lock is not recommended by apple, the document in apple's Threading Programming Guide:

A double-checked lock is an attempt to reduce the overhead of taking a lock by testing the locking criteria prior to taking the lock. Because double-checked locks are potentially unsafe, the system does not provide explicit support for them and their use is discouraged.`