Re: Multithreading

[Richard Frith-Macdonald]

On 8 Sep 2006, at 20:40, Wim Oudshoorn wrote:

> > I'm guessing that storing a single byte must be atomic on all
> > architectures, so how about this as a possible safe implementation?
> >
> > - (id) sharedInstance
> > {
> >    static volatile uint8_t      isCreated = 0;  // single byte
> >    declared  volatile so we read from memory rather than a register
> >    static volatile id theInstance = nil;
> >
> >    if (isCreated == 0)
> >      {
> >        [theLock lock];  
> >        if (isCreated == 0 && theInstance == nil)
> >          {
> >            theInstance = [theClass new];
> >          }
> >        [theLock unlock];
> >        isCreated = 1;
> >     }
> >    return theInstance;
> > }
>
> I think this should work.  But you could drop the isCreated == 0  
> clause
> from the second 'if'.  Because locks should ensure memory  
> consistency and
> that means that after obtaining the lock, either 'theInstance' is 0  
> or correct.

I see what you mean ... at that point,  if 'theInstance' is not nil  
then 'isCreated' must be 1, so they check of 'isCreated' is unnecessary.

> Now I will come back to something you mentioned in a previous e-mail:
>
> -[NSObject release] uses NSDecrementExtraRefCountWasZero before  
> deallocating,
> leaving a gap between decrementing and testing the refcount and  
> calling dealloc.
>
> Now this will create a problem, as such that the well known trick of
> writing your accessors as:
>
> - (id) getLatestError
> {
>   return AUTORELEASE (RETAIN (_errorMessage));
> }
>
> - (void) setLatestError: (NSString*) err
> {
>    ASSIGN (_errorMessage, err);
> }
>
> is NOT thread safe, contrary to what is normally claimed.
> (If thread A gets the error and increments the ref count just after
> thread B has decided it will dealocate it.)
>
> There are a few ways out of this:
>
> 1 - Ignore the problem.  Because it is quite unlikely to happen
> 2 - Unlearn the AUTORELEASE (RETAIN ()) trick and put locks
>     around all getters and setters which need to be thread safe.
> 3 - Change the DecRef and IncRef count is such a way that
>     the refcount will be -1 just before deallocating
>     and let retain return nil if the refcount is -1.
> 4 - the same as above, but let RETAIN throw an exception.
>
> Any thoughts?

Ah ... I thought it was well known that the AUTORELEASE (RETAIN ())  
sequence was not thread safe ... as I recall it being discussed on  
mailing lists (not sure whether it was on gnustep, apple or objc  
lists) some years ago.

This is not simply because of the gap between  decrementing the  
reference count and deallocating, but also because the ASSIGN() and  
RETAIN() are not atomic.
eg. if thread 1 is doing a RETAIN() and thread2 is doing an ASSIGN()
thread 2 calls -retain on a new value
thread 1 loads the old value of the ivar from memory
thread 2 loads the old value from memory
thread 2 stores the new value
thread 2 calls -release on the old value
thread 1 calls -retain on the old value ... but it has already been  
deallocated ... crash

My understanding is that the ASSIGN()/AUTORELEASE(RETAIN()) idiom is  
for thread safety in conjunction with locks
ie.

- (id) latestError
{
  id    result;
  [myLock lock];
  result = AUTORELEASE(RETAIN(_errorMessage));
  [myLock unlock];
  return result;
}

- (void) setLatestError: (NSString*)err
{
  [myLock lock];
  ASSIGN(_errorMessage, err);
  [myLock unlock];
}

So, of your set of four options, I think only 2 is OK.