CODEWITHSUNDEEP
c++multithreadingevent-handlingqueuecritical-section
kar
kar

Reputation: 495

critical section for a queue inside a thread

I have a singleton event class which has a queue and thread on it. The implementation is like, there are several modules/object which subscribes for certain event(s) and able to post multiple events.

The purpose of this event class is to receive all those event on push it on a queue and the thread inside this class needs to pop the event and send the same to the respective subscribed modules/objects.

The idea behind is creating a c++ bases EventAggregator which is available on C#.

The implementation is like 

    void c_eventAggregator::PostEvent(EVENT_UID EventUid)
{
    c_criticalRegion criticalRegion(eASyncObj);
    Queue.push_back(EventUid);
    criticalRegion.~c_criticalRegion();
}

void c_eventAggregator::DispatchEventToClients(EVENT_UID EventUid)
{
    EventClientsList eclist = _eventIdSubsList[EventUid];

    for (EventClientsList::iterator iter = eclist.begin(); iter != eclist.end(); iter++) {
        iter->second->receiveEvent(EventUid);
    }

}

int c_eventAggregator::SubscribeEvent(EVENT_CLIENTID clientId, c_eventClient *ecPtr, EVENT_UID EventUid)
{
    try
    {
        _eventIdSubsList[EventUid].insert(make_pair(clientId, ecPtr));
    }
    catch (int exception)
    {
        return exception;
    }
    return ZERO_VALUE;
}

void c_eventAggregator::run(void)
{
    EVENT_UID EventUid;

    while (isAlive())
    {
        while (Queue.size())
        {
            if (!Queue.empty())
            {
                c_criticalRegion criticalRegion(eASyncObj);
                EventUid = Queue[0];
                Queue.pop_front();

                DispatchEventToClients(EventUid);
                criticalRegion.~c_criticalRegion();
            }
        }
    }
}

I am using critical section between push and pop, so that queue doesn't get overwritten when multiple modules/object write at the same time. (Not sure whether this is correct).

My critical section handler is like

class c_criticalRegion{
  public:
    c_criticalRegion(c_syncObject &paSyncObject) : mSyncObject(paSyncObject){
      mSyncObject.lock();
    }

    ~c_criticalRegion(){
      mSyncObject.unlock();
    }

  private:
    c_syncObject &mSyncObject;

};

synchronization object is like sync.cpp,

c_syncObject::c_syncObject(){
    InitializeCriticalSection(&m_oMutexHandle);
}

c_syncObject::~c_syncObject(){
    DeleteCriticalSection(&m_oMutexHandle);
}

synch.h:

class c_syncObject{
private:
protected:
    //! The win32 CRITICAL_SECTION handle of the operating system.
    CRITICAL_SECTION m_oMutexHandle;

public:
    c_syncObject();
    ~c_syncObject();
    /*!\brief Lock the resource coming after the lock command
    *
    * This function blocks until it will get the lock for the coming critical section.
    */
    void lock(void){
        EnterCriticalSection(&m_oMutexHandle);
        //TODO handle return value
    };
    //!Free the resource coming after the lock command  
    void unlock(void){
        LeaveCriticalSection(&m_oMutexHandle);
        //TODO handle return value
    };
};

Issue I face here is the code doesn't work sometime unless I comment the criticalRegion.~c_criticalRegion() inside PostEvent(). Similarly, when there is an event send to PostEvent, the Queue inside the run() still shows size as zero.

This is kind of trivial where I end up getting the same situation on other files as well which has the similar kind of implementation.

Also I would like to know, when the release the critical section, after completing the task DispatchEventToClients() or before.

Upvotes: 0

Views: 553

Answers (1)

Michael Nastenko
Michael Nastenko

Reputation: 3093

You mustn't call destructor explicitly in this case. Actually, there are so little cases when you should call it, that you probably won't ever face one of them (I've only one in my life and it was a dirty hack).

Here is your problem:

void c_eventAggregator::PostEvent(EVENT_UID EventUid)
{
    c_criticalRegion criticalRegion(eASyncObj);
    Queue.push_back(EventUid);
    criticalRegion.~c_criticalRegion();    // <<< this is wrong
}

By this code criticalRegion destructor will be called twice - once by you and once by compiler.

This kind of classes called guards and they help you to avoid explicit call for "clean-up" (LeaveCriticalSection in your case). In your case it's not a big deal, but sometimes there are lot of returns in function and it's mess to put it everywhere. And explicit call won't be called in case of exception.

You can check simple example with explicit call and w/o explicit call

Upvotes: 1

Related Questions