Re: Another multithreading bug (I think)

[Richard Frith-Macdonald]

On 7 Sep 2006, at 10:56, Wim Oudshoorn wrote:

> Richard Frith-Macdonald <richard@tiptree.demon.co.uk> writes:
>
> > > 2 - the connection_table_gate is a non-recursive lock.
> >
> > Changed to be recursive.
> >
> > > 3 - the connection_table_gate should NOT change to a recursive lock
> > >     because it guards a hash table.
> >
> > That's not a problem because the table does not retain its contents,
> > so removing/adding an object has no side effects.
>
> Hm, well, the connection_table_gate is used to lock access to
> the connection_table.  connection_table is a NSHashTable and
> if the lock is recursive it can happen that during
> enumeration, by having side effects, indirectly a new connection
> will/can be added or removed.
> Of course it is a bug if this happens.  Like the +[NSConnection  
> _threadWillExit]
> bug I mentioned recently.   So if the lock is recursive such an
> connection_table usage bug will lead to a crash and in the other  
> case to a deadlock.
> I think the deadlock is slightly easier to debug (if the stacktrace  
> works at least.)

Sure ... what I was trying to say was that there are no side  
effects ... none that I can find anyway ... so a recursive lock would  
be OK.
However, I agree that it's nicer to use non-recursive locks where  
possible.

> > a more elegant solution here would be to scrap the special processing
> > in -release and have -dealloc do the work:
> >
> > 1. lock the connection table lock
> > 2. check the current retain count
> > 3a. ... if it is greater than 1, then something else has retained the
> > object ... so we cancel the dealloc and unlock
> > 3b. if it is 1, we remove the connection from the table, unlock, and
> > proceed to finalize/deallocate
>
> If it happens in dealloc you could conceivably use the _refGate lock.
> This is already recursive and an ivar, so you don't have threads  
> fighting
> for the global lock.

Unfortunately that wouldn't work ... since another thread could find  
the connection in the table while our thread is deallocating it, and  
then be left trying to retain a deallocated object.  We really have  
to prevent other threads from trying to use the object if we are  
deallocating it.

> > But ... I think this reveals a bug in NSObject ... the -retain and -
> > release implementations are not currently entirely thread-safe.  They
> > handle the testing and changing of the reference counts in a thread-
> > safe manner, but during release there is a gap between testing/
> > decrementing the reference count and calling the -dealloc method.
>
> Hm I see, yuck.

Not too difficult to fix I think.  I'm short of good test programs  
for threading though :-(