Is this code exceptions handled correctly?

Question

In the following code, it is possible that event throws exception and it may be not handled in even handler, (rare but its still the case)

I want keep "lck2" unlocked while executing the event, because I don't want main thread block for "mtx2", reason is nothing more than optimization.

Can I guarantee that "lck2" is always released in catch block? or there could be runtime exceptions and therefore it may cause deadlocks or some unexpected behavior?

std::unique_lock<std::mutex>lck2(mtx2); // lock used for waiting for event.

while (_isRunning) 
{
    try
    {
        while (_isRunning)
        {
            // cvar2 is condition variable
            cvar2.wait(lck2, [&] {return invoke; }); // wait until invoke == true

            if (invoke) // if event must be invoked
            {
                lck2.unlock();
                OnEvent(this, someproperty); // may throw exception
                lck2.lock();

                invoke = false; // execution completed
            }
        }
    }
    catch (...) // we need to keep this thread alive at all costs!
    {            
        lck2.lock(); // is this safe?
        invoke = false;
    }
}

Your `catch` block writes to `invoke` outside the mutex's protection — WhiZTiM, Jul 26 '17 at 08:49
At the moment of catching the exception `lck2` is always unlocked. — Hatted Rooster, Jul 26 '17 at 08:55
Your `cvar2.wait(lck2, [&]{ return invoke; })` will only return if `invoke` is `true`. What is the purpose of testing `invoke` again? — WhiZTiM, Jul 26 '17 at 09:23
I read something about Spurious wakeup, i didn't quite understand it but i thought adding if statement doesn't hurt too much. https://en.wikipedia.org/wiki/Spurious_wakeup @WhiZTiM — M.kazem Akhgary, Jul 26 '17 at 09:32
@M.kazemAkhgary That's the entire purpose of the second overload of [`std::condition_variable.wait(lock, lambda)`](http://en.cppreference.com/w/cpp/thread/condition_variable/wait): to prevent execution proceeding without the condition being satisfied. See my answer — WhiZTiM, Jul 26 '17 at 10:00

score 1 · Accepted Answer · answered Jul 26 '17 at 09:54

A rewrite of your code would probably be more appropriate, to make it easier for another developer to work on the code. I will show you two rewrites:

First, (Bad)

while (true)
{
    try
    {
        {
             std::lock_guard<std::mutex> lckx(mtx2);
             if(!_isRunning)
                   break;    //out of the main loop
        }

        bool should_invoke = false;
        {
                std::unique_lock<std::mutex> lck2(mtx2);
                cvar2.wait(lck2, [&] {return invoke; });
                should_invoke = invoke;
        }     

        if (should_invoke) // if event must be invoked
        {
            OnEvent(this, someproperty); // may throw exception
            {
                std::lock_guard<std:mutex> lckx(mtx2);
                invoke = false; // execution completed
            }
        }
    }
    catch (...) // we need to keep this thread alive at all costs!
    {            
        std::lock_guard<std:mutex> lckx(mtx2);
        invoke = false;
    }
}

Second, (Good)

Breaking the (first) code into smaller functional units; we also note that the expression cvar2.wait(lck2, [&]{ return invoke; }) will suspend execution and only return if woken up and invoke is true, then we can infer that we only need that expression to wait. Hence we can discard the superfluous use of invoke. Hence we have:

void do_work(){
    while(is_running()){
        try{
             wait_for_invocation();
             OnEvent(this, someproperty); // may throw exception
             set_invocation_state(false);
        catch(...){
             set_invocation_state(false);
        }
    }
}

Where the helpers are defined:

bool is_running(){
    std::lock_guard<std::mutex> lckx(mtx2);
    return _isRunning;
}

void wait_for_invocation(){
    std::unique_lock<std::mutex> lck2(mtx2);
    cvar2.wait(lck2, [&] {return invoke; });
}

void set_invocation_state(bool state){
    std::lock_guard<std::mutex> lckx(mtx2);
    invoke = state;
}

in your second code after `cvar2.wait` in second helper method `lck2` is owned again. i want it to be unlocked when firing `OnEvent` — M.kazem Akhgary, Jul 26 '17 at 10:35
I guess i got it, `unique_lock` also works like `lock_gaurd` when its destructed right? — M.kazem Akhgary, Jul 26 '17 at 10:37
doesn't creating `unique_lock` at every iteration hurt performance or memory? assuming that events can be fired 100 times per second. (at rate of 0.01Hz) — M.kazem Akhgary, Jul 26 '17 at 10:40
@M.kazemAkhgary, Yes... it releases the lock on destruction. See the documentation for [`std::unique_lock::~unique_lock`](http://en.cppreference.com/w/cpp/thread/unique_lock/%7Eunique_lock). Well, as regards to performance, don't speculate until you have measured. :-) — WhiZTiM, Jul 26 '17 at 13:20

Is this code exceptions handled correctly?

1 Answers1

First, (Bad)

Second, (Good)