Code execution stops on using thread safe Singleton initialization code

Question

To make use of global variables and methods I implemented Singleton as a healthy coding practice. I followed Apple documents, john wordsworth blog before implementing it. In the first, I did not make my singleton thread safe and I implemented this method along with all other mentioned in the blog and Apple document.

+ (SingletonClass *)sharedManager 
{
  static SingletonClass *sharedManager = nil;
  if (sharedManager == nil) {
    sharedManager = [[super allocWithZone:NULL] init];
}
  return sharedManager;
}

After that to make Singleton thread safe I made changes to + (SingletonClass *)sharedManager class like this and my app stops launching. I put break points and observed dispatch_once gets called twice and then code stops executing further.

+(SingletonClass *)sharedManager
{
  static SingletonClass *sharedManager = nil;
  if (sharedManager !=nil)
  {
    return sharedManager;
  }
  static dispatch_once_t pred;       
  dispatch_once(&pred, ^{
    sharedManager = [SingletonClass alloc];
    sharedManager=[sharedManager init];
});

     return sharedManager;
}

If i remove this thread safe code snippet and revert back to previous code it works fine and code gets executed.

Please note that I also looked at the bbum's answer here in which he has mentioned possible deadlock situation before asking question but I am not able to figure out the issue. Any explanation or solution will be helpful for me. Thanks.

Edit 1:

In case someone wants to look at the complete code, I have created gist for that. Please follow there. Thanks.

rob mayoff · Accepted Answer · 2013-01-07T08:55:57.983

Let's consider what happens if two threads call second version of sharedManager almost simultaneously.

Thread 1 calls first. It checks sharedManager !=nil, which is false, so it goes on to the dispatch_once. In the dispatch_once block, it executes [SingletonClass alloc] and stores the result in sharedManager.

Now, before thread 1 continues on to the next line, thread 2 comes along and calls sharedManager. Thread 2 checks sharedManager !=nil, which is now true. So it returns sharedManager, and the caller then tries to use sharedManager. But at this time, sharedManager hasn't been fully initialized yet. That's bad.

You cannot set sharedManager until you have a fully initialized object to set it to. Also (as borrrden pointed out), you don't need the sharedManager !=nil check at the top, because dispatch_once is very efficient anyway.

+ (SingletonClass *)sharedManager {
    static dispatch_once_t pred;
    static SingletonClass *sharedManager;
    dispatch_once(&pred, ^{
        sharedManager = [[SingletonClass alloc] init];
    });
    return sharedManager;
}

Now, I've looked at your gist and your problem is here:

+ (id)allocWithZone:(NSZone*)zone {
    return [[self sharedManager] retain];
}

Your +[SingletonClass sharedManager] method calls +[SingletonClass alloc] in the dispatch_once block. Since you don't override alloc, +[SingletonClass alloc] calls +[SingletonClass allocWithZone:NULL]. And +[SingletonClass allocWithZone:] method calls +[SingletonClass sharedManager]. On this second call to sharedManager, your program hangs in dispatch_once, because you're still inside the first call to dispatch_once.

The simplest fix is to remove your implementation of allocWithZone:. Just document that sharedManager is the only supported way to get an instance of SingletonClass and move on.

If you want to be obtuse and make [[SingletonClass alloc] init] return the singleton, even if you do it repeatedly, it's complicated. Don't try to override alloc or allocWithZone:. Do this:

static SingletonClass *sharedManager; // outside of any method

+ (SingletonClass *)sharedManager {
    return sharedManager ? sharedManager : [[SingletonClass alloc] init];
}

- (id)init {
    static dispatch_once_t once;
    dispatch_once(&once, ^{
        if (self = [super init]) {
            // initialization here...
            sharedManager = self;
        }
    });
    self = sharedManager;
    return self;
}

Thanks for detailed explanation of code.. I wrote `sharedManager = [[SingletonClass alloc] init];` in the first then only split it into two lines as per bbum's answer I have mentioned. You are right problem is somewhere else. Also posted a link of full code if someone wants to look in full code. — rohan-patel, Jan 07 '13 at 08:46
Great.. Working now.. Thanks for your time to look into full code. Accepting as an answer.. — rohan-patel, Jan 07 '13 at 08:53

score 1 · Answer 2 · answered Jan 07 '13 at 08:31

1

You don't need the check on the top, get rid of the if statement. The dispatch_once guarantees that the block will only be executed once in the lifetime of the application so the first check is redundant.

More Info: http://cocoasamurai.blogspot.jp/2011/04/singletons-your-doing-them-wrong.html

answered Jan 07 '13 at 08:31

borrrden

33,256
8
74
109

Worth adding: dispatch_once is fast. At least as fast as the "nil" check. The difference is that someone wrote some very clever code to make it thread safe, which the check for "nil" isn't. In the worst case, sharedSingleton could return nil with the code as written (if a second call comes in while sharedSingleton is still nil just before it is assigned, the running dispatch_once finishes, dispatch_once does nothing because the first dispatch_once has finished, and because sharedSingleton is not volatile the compiler assumes it is still nil and returns nil. – gnasher729 Mar 04 '14 at 19:09

Code execution stops on using thread safe Singleton initialization code

2 Answers2

Linked