5

There are tons of questions on how to implement thread safe reference counters. And a common highly voted answer is: "use atomic increment/decrements". Ok, this is a good way to read and write refCounter whitout other thread changing it in between. But.

My code is:

void String::Release()
{
    if ( 0 == AtomicDecrement( &refCounter ) ) )
        delete buffer;
}

So. I decrement and read refCounter in safe. But what if other thread will INCREMENT my refCounter while I am comparing it to zero????

Am I wrong?

EDIT: (example)

String* globalString = new String(); // refCount == 1 after that.

// thread 0:
delete globalString; 
  // This invokes String::Release().
  // After AtomicDecrement() counter becomes zero. 
  // Exactly after atomic decrement current thread switches to thread 1.

// thread 1:
String myCopy = *globalString;
  // This invokes AddRef(); 
  // globalString is alive;
  // internal buffer is still not deleted but refCounter is zero;
  // We increment and switch back to thread 0 where buffer will be 
  // succefully deleted;

Am I wrong?

skaffman
  • 398,947
  • 96
  • 818
  • 769
Anton Petrov
  • 782
  • 1
  • 8
  • 19
  • 2
    How can another thread increment the counter if it doesn't have a reference to the object? A value of 0 literally means "no references left". – Hans Passant Feb 08 '11 at 15:33

2 Answers2

2

Be careful !

It is not enough to protect a variable like a reference counter that manage the life cycle of something bigger.

I've seen code like the one in your question that ends up pretty bad ...

In your case is not only that someone could increment the counter after your comparison, but some thread can get the counter with value 1, then you decrement and DELETE the buffer and the other thread use deleted memory ... CRASH

my2c

neuro
  • 14,948
  • 3
  • 36
  • 59
1

Your example sounds right to me.

However, the issue here is not about atomic operations, but manually deleting an object and then referencing a soon-to-be-deleted object. What if the reference count, instead of being 1, is 8?.

You need to avoid deleting and invalidating the object manually, and better use some smart pointers implementation aware of concurrency to handle the reference counting.

Whenever a pointer detects the refcount to be zero you need to lock the object to avoid being referenced by other thread, much like the double-checked locking for initializing the new reference.

vz0
  • 32,345
  • 7
  • 44
  • 77
  • 2
    How would another thread access the object, when there are no references to the object left? – Jeremy Friesner Feb 08 '11 at 16:40
  • 1
    @Jeremy, As Anton said on the question example, if one thread calls the Release() method and, just after the AtomicDecrement call and before the delete statement another thread tries to acquire a reference, the refcount would equal to zero but the secondth thread would get the reference, incrementing the refcount just before the delete, deleting an object with a refcount == 1. It's a highly-improbable-yet-not-impossible case. – vz0 Feb 08 '11 at 17:01
  • Thank you people. It is actually a problem of making globalString thread-safe and not the internal buffer. So as vz0 suggested I should lock the globalString first before attempting to work with it. P.S. String is just an example, in my real code I have a bit more complicated dependencies. – Anton Petrov Feb 09 '11 at 07:41