views:

133

answers:

3

I have these two methods for thread-exclusive access to a CMyBuffer object:

Header:

class CSomeClass
{
//...
public:
    CMyBuffer & LockBuffer();
    void ReleaseBuffer();

private:
    CMyBuffer m_buffer;
    CCriticalSection m_bufferLock;
//...
}

Implementation:

CMyBuffer & CSomeClass::LockBuffer()
{
    m_bufferLock.Lock();
    return m_buffer;
}

void CSomeClass::ReleaseBuffer()
{
    m_bufferLock.Unlock();
}

Usage:

void someFunction(CSomeClass & sc)
{
    CMyBuffer & buffer = sc.LockBuffer();
    // access buffer
    sc.ReleaseBuffer();
}
  • What I like about this is, that the user has to make only one function call and can only access the buffer after having locked it.
  • What I don't like is that the user has to release explicitly.

Update: These additional disadvantages were pointed out by Nick Meyer and Martin York:

  • The user is able to release the lock and then use the buffer.
  • If an exception occurs before releasing the lock, the buffer remains locked.

I'd like to do it with a CSingleLock object (or something similar), which unlocks the buffer when the object goes out of scope.

How could that be done?

+1  A: 

IMHO, if your goal is to prevent the user from only accessing the buffer when it is locked, you're fighting a tricky battle. Consider if the user does:

void someFunction(CSomeClass & sc)
{
   CMyBuffer & buffer = sc.LockBuffer();
   sc.ReleaseBuffer();
   buffer.someMutatingMethod(); // Whoops, accessed while unlocked!
}

In order to allow the user access to the buffer, you've got to return a reference to the buffer, which they can always make the mistake of holding on to until after the lock is released.

That said, you may be able to do something like this:

class CMyBuffer
{
   private:
      void mutateMe();
      CCriticalSection m_CritSec;

   friend class CMySynchronizedBuffer;
};

class CMySynchronizedBuffer
{
   private:
      CMyBuffer & m_Buffer;
      CSingleLock m_Lock

   public:
      CMySynchronizedBuffer (CMyBuffer & buffer)
         : m_Buffer (buffer)
         , m_Lock (&m_Buffer.m_CritSec, TRUE)
      {
      }

      void mutateMe()
      {
         m_Buffer.mutateMe();
      }
};

Use like:

{
   CMyBuffer buffer;  // Or declared elsewhere
   // buffer.mutateMe();  (Can't do this)
   CMySyncrhonizedBuffer synchBuffer (buffer); // Wrap it & lock it
   synchBuffer.mutateMe();  // Now protected by critical section
} // synchBuffer and its CSingleLock member are destroyed and the CS released

This forces the user to wrap the CMyBuffer object in a CMySynchronizedBuffer object in order to get at any of its mutating methods. Since it doesn't actually provide access to the underlying CMyBuffer object by returning a reference, then it shouldn't give the user anything to hang onto and mutate after the lock is released.

Nick Meyer
+2  A: 

One way to do this would be using RAII:

class CMyBuffer
{
public:
    void Lock()
    {
     m_bufferLock.Lock();
    }

    void Unlock()
    {
     m_bufferLock.Unlock();
    }

private:
    CCriticalSection m_bufferLock;

};

class LockedBuffer
{
public:
    LockedBuffer(CMyBuffer& myBuffer) : m_myBuffer(myBuffer)
    {
     m_myBuffer.Lock();
    }

    ~LockedBuffer()
    {

     m_myBuffer.Unlock();
    }

    CMyBuffer& getBuffer() 
    {
     return m_myBuffer;
    }

private:
    CMyBuffer& m_myBuffer;

};

class CSomeClass
{
    //...
public:
    LockedBuffer getBuffer();

private:
    CMyBuffer m_buffer;

};


LockedBuffer CSomeClass::getBuffer()
{
    return LockedBuffer(m_buffer);
}

void someFunction(CSomeClass & sc)
{
    LockedBuffer lb = sc.getBuffer();
    CMyBuffer& buffer = lb.getBuffer();
    //Use the buffer, when lb object goes out of scope buffer lock is released
}
Naveen
Nick Meyer
@Nick: So could the original code. At least this version protects against problems with exceptions.
Martin York
+1  A: 

Use an object that represents the buffer.
When this obejct is initialized get the lock and when it is destroyed release the lock.
Add a cast operator so it can be used in place of the buffer in any function call:

#include <iostream>

// Added to just get it to compile
struct CMyBuffer
{    void doStuff() {std::cout << "Stuff\n";}};
struct CCriticalSection
{
        void Lock()     {}
        void Unlock()   {}
};          

class CSomeClass
{
    private:
        CMyBuffer m_buffer;
        CCriticalSection m_bufferLock;

        // Note the friendship.
        friend class CSomeClassBufRef;
};

// The interesting class.
class CSomeClassBufRef
{
    public:
        CSomeClassBufRef(CSomeClass& parent)
            :m_owned(parent)
        {
           // Lock on construction
            m_owned.m_bufferLock.Lock();
        }
        ~CSomeClassBufRef()
        {
            // Unlock on destruction
            m_owned.m_bufferLock.Unlock();
        }
        operator CMyBuffer&()
        {
            // When this object needs to be used as a CMyBuffer cast it.
            return m_owned.m_buffer;
        }
    private:
        CSomeClass&     m_owned;
}; 

void doStuff(CMyBuffer& buf)
{           
    buf.doStuff();
}
int main()
{
    CSomeClass          s;

    // Get a reference to the buffer and auto lock.
    CSomeClassBufRef    b(s);

    // This call auto casts into CMyBuffer
    doStuff(b);

    // But you can explicitly cast into CMyBuffer if you need.
    static_cast<CMyBuffer&>(b).doStuff();
}
Martin York
What do you think of overloading **operator->()**? This way one could say *b->doStuff()* instead of having to make the *static_cast*.
mxp
Addendum: I think the *static_cast* has been made so ugly in order to make it eye-catching and not-so-trivial to cast something. But in this case it should actually be done that way, so there is no need to make this cast harder than necessary for the user.
mxp
I'm currently trying it out and noticed, the **operator*()** should also be overloaded then.
mxp
Personally I would not defined operator*() or operator->(). I would use the cast operator or potentially a get() method (on the CSomeClassBufRef)
Martin York