Thread-safe lazy get and release

Question

Hello All,

I'm running into kindof an annoying problem and would need some advice...

Let's say I have a bunch of small MyObject's, that can construct bigger MyExtendedObject's. MyExtendedObject's are big and CPU consuming so construction is lazy, and I try do remove them from memory as soon as possible:

MyExtendedObject * MyObject::GetExtentedObject(){
  if(NULL == ext_obj_){
    ext_obj_ = new MyExtendedObject;
  }
  ++ref_;
  return ext_obj_;
}
void MyObject::ReleaseExtentedObject(){
  if(0 == (--ref_))
  {
    if(NULL != ext_obj_)
    {
      delete ext_obj_;
      ext_obj_ = NULL;
    }
  }
}

Extended objects are only constructed once at the beginning and are destroyed when the last caller releases them. Note that some may be constructed more than once but this is not an issue here.

Now, this is absolutely not thread-safe so I did a "naive" thread-safe implementation:

MyExtendedObject * MyObject::GetExtentedObject(){
  Lock();
  if(NULL == ext_obj_){
    ext_obj_ = new MyExtendedObject;
  }
  ++ref_;
  Unlock();
  return ext_obj_;
}
void MyObject::ReleaseExtentedObject(){
  Lock();
  if(0 == (--ref_))
  {
    if(NULL != ext_obj_)
    {
      delete ext_obj_;
      ext_obj_ = NULL;
    }
  }
  Unlock();
}

This is better but now I spend some non neglectable amount of time locking and unlocking...

I had the feeling we could pay the lock/unlock only when constructing or destructing.

I came up with this solution:

MyExtendedObject * MyObject::GetExtentedObject(){
  long addref = InterlockedCompareExchange(&ref_, 0, 0);
  long result;
  do{
    result = addref + 2;
  } while ((result-2) != (addref = InterlockedCompareExchange(&ref_, result, addref)));
  if(0 == (result&1)){
    Lock();
    if(NULL == ext_obj_){
      ext_obj_ = new MyExtendedObject;
      InterlockedIncrement(&ref_);
    }
    Unlock();
  }
  return ext_obj_;
}
void MyObject::ReleaseExtentedObject(){
  long release = InterlockedCompareExchange(&ref_, 0, 0);
  long result = 0;
  do{
    result = release - 2;
  } while ((result+2) != (release = InterlockedCompareExchange(&ref_, result, release)));
  if(1 == result)
  {
    Lock();
    if(1 == InterlockedCompareExchange((long*)&ref_, 0, 1))
    {
      if(NULL != ext_obj_)
      {
        delete ext_obj_;
        ext_obj_ = NULL;
      }
    }
    Unlock();
  }
}

Some explanations:

• I cannot use Boost. I'd like to but really cannot.

• I use only CompareExchange and Incr / Decr on purpose. Don't ask.

• I use first bit of ref_ to store the construction status (constructed / not constructed) and other bits for ref counting. It's the only way I found to manage 2 variables (ref counting and construction status) at the same time through atomic operations.

Some questions now :

• Do you think it's 100% bullet proof?

• Do you know some better solutions?

EDIT: Some have proposed to use shared_ptr. One up for a working solution with shared_ptr! Note that I need: lazy construction AND destruction when nobody no more use it.

Answer 1

It sounds like you are partway to reconstructing boost::shared_ptr, which offers reference counting of an object via an encapsulated raw pointer.

In your case usage would be boost::shared_ptr<MyExtendedObject>.

EDIT: Per @ronag's comment, shared_ptr is now supported natively by many current compilers after being accepted into the latest version of the language.

EDIT: First time construction:

shared_ptr<MyExtendedObject> master(new MyExtendedObject);

When the last copy of master goes out of scope, delete MyExendedObject will be called.

Answer 2

As Steve said, you basically want shared_ptr for the construction/destruction part. If you can't use boost, then I'd recommend copying the appropriate code from the boost headers (I believe the license allows this), or whatever other workaround you need to circumvent your dumb corporate policies. The other advantage of this approach is that when you can adopt TR1 or C++0x, you won't need do rewrite/maintain any custom implementation, you can just use the [then] built-in library code.

As for the thread safety (which Steve didn't address), I find it's almost always a good idea to use synchronization primitives rather than trying to get it right yourself with custom locking. I'd suggest using a CRITICAL_SECTION, and then adding some timing code to make sure the total lock/unlock time is negligible. Doing a lot of lock/unlock operations is fine, as long as there's not too much contention, and you won't have to debug obscure threaded access issues.

That's my advice, anyway, FWIW.

Edit: I should add that once you're effectively using boost, you'll probably want to keep a weak_ptr in the MyObject class, so you can check if the extended object still exists on the "get" function without holding a reference to it. This will allow your "ref counted destruction" when no external caller is still using the instance. So, your "get" function looks like:

shared_ptr< MyExtendedObject > MyObject::GetExtentedObject(){
  RIIALock lock( my_CCriticalSection_instance );
  shared_ptr< MyExtendedObject > spObject = my_weak_ptr.lock();
  if (spObject) { return spObject; }

  shared_ptr< MyExtendedObject > spObject = make_shared< MyExtendedObject >();
  my_weak_ptr = spObject;
  return spObject;
}

... and you don't need a release function, cause that part is done automatically through shared_ptr's reference counting. Hope that's clear.

See: http://stackoverflow.com/questions/2160334/boost-weak-ptrs-in-a-multi-threaded-program-to-implement-a-resource-pool for more info on weak_ptr and threading safety.