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?

"Obviously two sNetworkManager may be created. So Apple made a mistake, right?" Why? — Stefan H, Apr 06 '12 at 06:03

score 2 · Answer 1 · edited May 23 '17 at 12:01

2

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.`

edited May 23 '17 at 12:01

Community

1
1

answered Apr 07 '12 at 06:10

tangqiaoboy

1,476
1
15
32

i think double check lock is not correct and not supported on both iOS an Mac. – Bob Cromwell Apr 09 '12 at 03:29
@BobCromwell Why? Could you give me some reason or reference? – tangqiaoboy Apr 11 '12 at 03:45
1

in simple words, the sNetwokManager could be not nil before the object is fully init-ed. Please search Double-checkedlock in Apple's document "Threading Programming Guide" and here is a post about this: http://www.wincent.com/a/knowledge-base/archives/2006/01/locking_doublec.php – Bob Cromwell Apr 11 '12 at 05:59
@BobCromwell I've found the content in apple's doc: `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.` You're right, thank you. – tangqiaoboy Apr 11 '12 at 06:51

score 1 · Accepted Answer · edited May 23 '17 at 11:46

Yes, it's wrong. Start with sNetworkManager as nil, and consider two threads T1 and T2.

One possible, if unlikely, scenario is:

T1: Determines (sNetworkManager == nil) is true
T2: Determines (sNetworkManager == nil) is true
T1: Takes the @synchronized lock
    Creates a NetworkManager 
    Sets sNetworkManager
    Releases the lock
T2: Takes the @synchronized lock
    Creates a NetworkManager 
    Sets sNetworkManager, LEAKING the first one
    Releases the lock

This question has some safer ways of doing it.

score 0 · Answer 3 · answered Apr 06 '12 at 06:13

0

There is no mistake in this code. Only one sNetworkManager is created for the simple reason that the word "static" is used. The static keyword is used here to define the variable as global but only visible to that function. The variable is allocated in the first call of + (NetworkManager *)sharedManager then it's no more to null and not initialized any more.

answered Apr 06 '12 at 06:13

user1316852

1

Hi Guy,have a look at Kurt's answer,could you please add your comments there if you think Kurt's analysis is not correct. – Bob Cromwell Apr 06 '12 at 07:13
His analysis is true but it's unlikely that this scenario occurs. Would you speak about this thread safe mistake in your first message? I thought you speak about the "static" keyword. – user1316852 Apr 19 '12 at 03:49

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

3 Answers3