incorrect ref counting for timer queues

It appears that this bug was introduced by the fix for mantis 332 on 2009-2-10

Perhaps we need a fix like this:

before:

void timerQueueActiveMgr ::
    release ( epicsTimerQueueActiveForC & queue )
{
    timerQueueActiveMgrPrivate * pPriv = & queue;
    {
        Guard locker ( this->mutex );
        assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
        queue.timerQueueActiveMgrPrivate::referenceCount--;
        if ( queue.timerQueueActiveMgrPrivate::referenceCount == 0u ) {
            if ( queue.sharingOK () ) {
                this->sharedQueueList.remove ( queue );
            }
        }
    }
    // delete only after we release the guard in case the embedded
    // reference is the last one and this object is destroyed
    // as a side effect
    delete pPriv;
}

after:

void timerQueueActiveMgr ::
    release ( epicsTimerQueueActiveForC & queue )
{
    timerQueueActiveMgrPrivate * pPriv = & queue;
    {
        Guard locker ( this->mutex );
        assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
        queue.timerQueueActiveMgrPrivate::referenceCount--;
        if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
            pPriv = 0;
        }
        else if ( queue.sharingOK () ) {
            this->sharedQueueList.remove ( queue );
        }
    }
    // delete only after we release the guard in case the embedded
    // reference is the last one and this object is destroyed
    // as a side effect
    idelete pPriv;
}

Hi Jeff,

I think this would still delete the queue before the count is zero. Try
putting:

assert ( queue.timerQueueActiveMgrPrivate::referenceCount == 0u );

before the delete. I think that fundumentally the delete must be in a
conditional, or a second return path is needed.

On 05/24/11 20:10, Jeff Hill wrote:
> It appears that this bug was introduced by the fix for mantis 332 on
> 2009-2-10
>
> Perhaps we need a fix like this:

...

> after:
>
> void timerQueueActiveMgr ::
> release ( epicsTimerQueueActiveForC & queue )
> {
> timerQueueActiveMgrPrivate * pPriv = & queue;
> {
> Guard locker ( this->mutex );
> assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
> queue.timerQueueActiveMgrPrivate::referenceCount--;
> if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
> pPriv = 0;
> }
> else if ( queue.sharingOK () ) {
> this->sharedQueueList.remove ( queue );
> }
> }
> // delete only after we release the guard in case the embedded
> // reference is the last one and this object is destroyed
> // as a side effect
> idelete pPriv;
> }
>

Hi David,

> I think this would still delete the queue before the count is zero

Note that there is the if statement which sets pPriv to nill, and of course the delete will not fire if pPriv is nill

        if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
            pPriv = 0;
        }

> I think that fundamentally the delete must be in a conditional, or a second return path is needed.

Note that the delete can not occur when the guard is still active. So we determine if the delete will need to occur when we have the lock, and then defer it until after the guard has been released

On 5/26/2011 10:23 AM, Jeff Hill wrote:
> Hi David,
>
>> I think this would still delete the queue before the count is zero
> Note that there is the if statement which sets pPriv to nill, and of
> course the delete will not fire if pPriv is nill
>
> if ( queue.timerQueueActiveMgrPrivate::referenceCount> 0u ) {
> pPriv = 0;
> }

Ok, I see. I would suggest replacing "pPriv = 0;" with "return;" since
I think this makes it more obvious what is going on...

>> I think that fundamentally the delete must be in a conditional, or a
> second return path is needed.
>
> Note that the delete can not occur when the guard is still active. So we
> determine if the delete will need to occur when we have the lock, and
> then defer it until after the guard has been released
>

ok with that mod

I built here with this version and epicsTimerTest passes, but my version does not have your new test

   {
        Guard locker ( this->mutex );
        assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
        queue.timerQueueActiveMgrPrivate::referenceCount--;
        if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
            return;
        }
        else if ( queue.sharingOK () ) {
            this->sharedQueueList.remove ( queue );
        }
    }
    // delete only after we release the guard in case the embedded
    // reference is the last one and this object is destroyed
    // as a side effect
    timerQueueActiveMgrPrivate * const pPriv = & queue;
    delete pPriv;

>bzr diff timerQueueActiveMgr.cpp
=== modified file 'src/libCom/timer/timerQueueActiveMgr.cpp'
--- src/libCom/timer/timerQueueActiveMgr.cpp 2011-01-15 01:59:34 +0000
+++ src/libCom/timer/timerQueueActiveMgr.cpp 2011-05-26 21:24:51 +0000
@@ -57,20 +57,21 @@
 void timerQueueActiveMgr ::
     release ( epicsTimerQueueActiveForC & queue )
 {
- timerQueueActiveMgrPrivate * pPriv = & queue;
     {
         Guard locker ( this->mutex );
         assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
         queue.timerQueueActiveMgrPrivate::referenceCount--;
- if ( queue.timerQueueActiveMgrPrivate::referenceCount == 0u ) {
- if ( queue.sharingOK () ) {
- this->sharedQueueList.remove ( queue );
- }
+ if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
+ return;
+ }
+ else if ( queue.sharingOK () ) {
+ this->sharedQueueList.remove ( queue );
         }
     }
- // delete only after we release the guard in case the embedded
+ // delete only after we release the guard in case the embedded
     // reference is the last one and this object is destroyed
     // as a side effect
+ timerQueueActiveMgrPrivate * const pPriv = & queue;
     delete pPriv;
 }

Ok, looks like this works. I'm attaching an updated patch against the
3.14 branch.