Possible deadlock situation in the wait method?

Question

The Test class is used in a multithreaded enviroment. ThreadA asks if he has to wait for ThreadB by calling the hasToWait method (ITestWaiter). When ThreadB has done his work he notify`s all waiters by calling the Test::notify method.

Could you tell me if there is a possible deadlock situation in the wait() method - between the part, which is locked by the mutex and the call to the semaphore acquire method?

struct Semaphore {
  bool acquire() { return WaitForSingleObject(sem, INFINITE); }
 private:
  Handle sem;
};

struct Test
{
  bool wait(std::mutex mutex, const ITestWaiter *obj);
  bool notify(std::mutex mutex);

private:
  std::vector<Semaphore> waiters;
};

bool Test::wait(std::mutex mutex, const ITestWaiter *obj) {
  Semaphore* sem;
  {
    std::unique_lock<std::mutex> mlock(mutex);
    if (!obj->hasToWait())
      return false;

    sem = createSemaphoreAndPushBackToVector();
 }
 try {
   sem->acquire();
 }
 catch (std::exception e) {}

 return true;
}

bool Test::notify(std::mutex mutex) {
  std::unique_lock<std::mutex> mlock(mutex);
  //notify waiters by releasing the semaphore
  return true;
}

Aconcagua · Accepted Answer · 2019-02-26T11:00:37.930

0

From the code you posted, there shouldn't be a problem: In both cases, you do not block during the time you hold the lock; you just do some small actions (once modify the vector, once iterate over it) instead. But there's code you didn't show!

First, there's how you are going to notify. I assume you use CreateEvent to get the handle and SetEvent for notification – if so, no problem either.

Then, there's the hasToWait function. Suspicious: You are calling it while already holding the lock! Is there any reason for? Does hasToWait some locking, too? Does the other thread possibly try to lock the same facility? Then risk of deadlock exists if both threads do not acquire the locks in the same order.

If there's no separate locking involved, but hasToWait needs to access some resources that need to be protected by the same mutex, then the code as is is fine, too.

If there's no locking and no access to shared resources, then locking the mutex first is in vain and just requires time; in this case, first checking is more efficient:

if (obj->hasToWait())
{
    Semaphore* sem;
    {
        std::unique_lock<std::mutex> mlock(mutex);
        sem = createSemaphoreAndPushBackToVector();
    }
    try
    {
        sem->acquire();
    }
    catch (std::exception e)
    { }
}

edited Feb 26 '19 at 11:00

answered Feb 26 '19 at 10:26

Aconcagua

24,880
4
34
59

Thanks for your answer. In the notify() method I`m using ReleaseSemaphore() method for each waiter in the list and pop the waiters. In the hasToWait() method I check if the thread has to wait for the other thread. The method locks the same lock again but no other lock. Is this a problem? – user5580578 Feb 26 '19 at 11:27
`ReleaseSemaphore` is fine as well. `std::mutex` is *not* re-entrant, so locking it again if already held *will* result in deadlock! If you want to do so, you need a [recursive_mutex](https://en.cppreference.com/w/cpp/thread/recursive_mutex). – Aconcagua Feb 26 '19 at 11:33
Thanks for the hint. Instead of using a recursive_mutex can I also use a critical section? – user5580578 Feb 26 '19 at 12:23
@user5580578 According to [here](https://stackoverflow.com/questions/7260432/is-it-valid-to-nest-a-critical-section), yes. I personally would prefer standard library over OS-specific facilities, though, your code gets more portable this way. – Aconcagua Feb 26 '19 at 12:55

Possible deadlock situation in the wait method?

1 Answers1