Re: Question about memory management

[David Chisnall]

On 7 Jun 2013, at 09:34, Maxthon Chan <address@hidden> wrote:

> I don't want an exclusive singleton - that is, there is not only one shared 
> singleton instance

Exclusive singleton is a tautology.  By definition, singletons are exclusive.

> , the user can also set up one for their own, like recent versions of 
> NSFileManager on OS X.
> 
> Can I do this:
> 
> // Singleton.m
> static Singleton *__singleton
> 
> @implementation Singleton
> + (instancetype)sharedSingleton
> {
>       @synchronized (__singleton)
>       {
>               if (!__singleton)
>                       __singleton = [[self alloc] init];
>       }

This is both inefficient (you need to acquire the lock on every entry into the 
method, which requires two calls into the runtime, some atomic operations, and 
so on) and wrong.  You are locking on __singleton (which is initially nil, so 
will probably crash as @synchronized(nil) is undefined), not on something that 
is guaranteed to be shared, such as the class.  

Given that +initialize does all of the required serialisation, however, there 
is no even approximately sane reason to write this less efficient, more 
fragile, and more verbose version.

Oh, and in C identifiers beginning with a double underscore are reserved for 
the implementation and so even with correct locking this would be undefined 
behaviour (a compliant compiler would be free to define variables called 
__singleton as variables that always return 0 when read.  More plausibly, it 
would be free to use __singleton as an annotation to assist with static 
analysis, so you'd be calling a variable a keyword reserved as a type 
qualifier).

If you want it to be possible to create multiple instances, along the lines of 
NSFileManager, then move the assignment to DefaultSingleton into +initialize 
and remove the +allocWithZone: method entirely, so you're left with:

+ (void)initialize
{
        if (self == [Singleton class])
        {
                DefaultSingleton = [[self alloc] init];
        }
}


>       return __singleton;
> }
> // …
> @end
> 
> 在 2013-6-7，下午4:26，David Chisnall <address@hidden> 写道：
> 
>> On 7 Jun 2013, at 07:29, Maxthon Chan <address@hidden> wrote:
>> 
>>> Just asking, with ARC, is this a good choice on implementing singleton?
>> 
>> No, it's not thread-safe.
>> 
>>> //Singleton.h
>>> #import <Foundation/Foundation.h>
>>> @interface Singleton : NSObject
>>> + (instancetype)defaultSingleton;
>>> // …
>>> @end
>>> extern Singleton *DefaultSingleton // Of course this is optional
>>> 
>>> // Singleton.m
>>> Singleton *DefaultSingleton
>> 
>> This should be declared static, as the variable should not be exposed 
>> outside of the Singleton.m compilation unit.
>> 
>>> @implementation Singleton
>>> + (instancetype)defaultSingleton
>>> {
>>>     if (!DefaultSingleton)
>>>             DefaultSingleton = [[self alloc] init];
>> 
>> If two threads call this method at once, then both will enter the body of 
>> this if statement, both will allocate instances of the singleton and one 
>> version will leak.
>> 
>> The correct way of implementing a singleton is to create it in the 
>> +initialize method, which is guaranteed to be thread-safe.  My preferred 
>> pattern is:
>> 
>> + (void)initialize
>> {
>>      [[self alloc] init];
>> }
>> + allocWithZone: (NSZone*)aZone
>> {
>>      if (DefaultSingleton != nil)
>>      {
>>              [NSException raise: NSInvalidArgumentException format: 
>> @"Attempted to create multiple instances of singleton %@", self];
>>      }
>>      DefaultSingleton = [super allocWithZone: aZone];
>>      NSAssert(nil != DefaultSingleton, @"Allocation of singleton instance of 
>> %@ failed!", self);
>>      return DefaultSingleton;
>> }
>> 
>> Calling the constructor in +initialize means that the first time ANY message 
>> is sent to this class, the singleton will be created and no other thread 
>> will be allowed to send messages to this class until the singleton is fully 
>> initialised.
>> 
>> The nil check in +allocWithZone: is safe, because the variable will be set 
>> with a lock held and can then be safely queried because it will never 
>> transition from non-nil to nil.  This exception prevents people from calling 
>> +alloc on the object.
>> 
>> Assigning the value to DefaultSingleton in +allocWithZone: instead of 
>> +initialize means that calls to things from the -init method don't have to 
>> have special cases to check the presence of the singleton.  
>> 
>> For completeness, you may also include NSCoding methods that ensure that the 
>> singleton is correctly created when serialising / deserialising.  It would 
>> be nice if there were an NSSingleton protocol that included a 
>> +sharedInstance method so that the serialiser code could know to always 
>> replace any references to the class with the shared instance, but there 
>> isn't.  
>> 
>> David
>> 
> 
> 
> _______________________________________________
> Gnustep-dev mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/gnustep-dev


-- Sent from my Difference Engine