Is it bad practice to call notify_one()/notify_all() while holding the lock on the mutex?

Question

I was writing a practice program to understand multithreading concepts in C++. This program simply reads a string, input from the user on one thread and processes it in another.

1   #include <iostream>
2   #include <thread>
3   #include <condition_variable>
4   #include <mutex>
5   #include <queue>
6   #include <string>
7   
8   #define __DEBUG
9   
10  #ifdef __DEBUG
11  #define PRINT cout << __FUNCTION__ << " --- LINE : " << __LINE__ << '\n'
12  #else
13  #define PRINT
14  #endif
15  
16  using namespace std;
17  
18  condition_variable g_CondVar;
19  mutex g_Mutex;
20  queue<string> g_Queue;
21  
22  void AddTaskToQueue()
23  {
24      string InputStr;
25      while (true)
26      {
27          lock_guard<mutex> LG(g_Mutex);
28          PRINT;
29          cin >> InputStr;
30          PRINT;
31          g_Queue.push(InputStr);
32          PRINT;
33          g_CondVar.notify_one();
34          PRINT;
35          if (InputStr == "Exit")
36              break;
37          this_thread::sleep_for(50ms);
38      }
39  }
40  
41  void ProcessQueue()
42  {
43      PRINT;
44      unique_lock<mutex> UL(g_Mutex);
45      PRINT;
46      string ProcessingStr;
47      PRINT;
48      while (true)
49      {
50          PRINT;
51          g_CondVar.wait(UL, [] {return !g_Queue.empty(); });
52          PRINT;
53          ProcessingStr = g_Queue.front();
54          cout << "Processing ----" << ProcessingStr << "----" << '\n';
55          PRINT;
56          g_Queue.pop();
57          PRINT;
58          if (ProcessingStr == "Exit")
59              break;
60          this_thread::sleep_for(50ms);
61      }
62  }
63
64  int main()
65  {
66      thread TReadInput(AddTaskToQueue);
67      thread TProcessQueue(ProcessQueue);
68  
69      TReadInput.join();
70      TProcessQueue.join();
71  }

The output is as below.

AddTaskToQueue --- LINE : 28
ProcessQueue --- LINE : 43
TestString
AddTaskToQueue --- LINE : 30
AddTaskToQueue --- LINE : 32
AddTaskToQueue --- LINE : 34
AddTaskToQueue --- LINE : 28

I have few queries which I don't want to conclude myself/ I cannot understand.

1. Notice from the output that the lock is never acquired on Line: 44, why is it so?
2. Is it a good practice to call notify_one()/notify_all() while the mutex is locked as in Line: 33.
3. Are there any chances that thread TProcessQueue might start executing before TReadInput? I mean to ask if the execution order of n threads would be the same as how they are instantiated?
4. Should the mutex be locked before calling wait on the condition_variable?. I mean to ask if the below code is okay?

//somecode
unique_lock<mutex> UL(Mutex, defer_lock);  //Mutex is not locked here
ConditionVariable.wait(UL, /*somecondition*/);
//someothercode

5. If I change Line: 44 to unique_lock<mutex> UL(g_Mutex, defer_lock); The output is as below and there is an exception thrown on Line: 38

AddTaskToQueue --- LINE : 28
ProcessQueue --- LINE : 43
ProcessQueue --- LINE : 45
ProcessQueue --- LINE : 47
ProcessQueue --- LINE : 50
TestString
AddTaskToQueue --- LINE : 30
AddTaskToQueue --- LINE : 32
AddTaskToQueue --- LINE : 34
ProcessQueue --- LINE : 52
Processing ----TestString----
ProcessQueue --- LINE : 55
ProcessQueue --- LINE : 57
d:\agent\_work\1\s\src\vctools\crt\crtw32\stdcpp\thr\mutex.c(175): unlock of unowned mutex
ProcessQueue --- LINE : 50

Why does this happen and what is unlock of unowned mutex?

6. If you notice any better programming practice I should have used in any other part of this code please point out.

Answer 1

1. Your biggest problem is at the moment most likely that your threads sleep while holding the mutex. That means the only time a different thread can take ownership of the mutex is in the tiny time window after 50ms when the mutex is shortly released and reacquired. You should fix this, and might see different results.
2. It's preferable to call notify outside of the lock. The reason for this is that the woken thread might immediately start running from there, but then gets blocked on the mutex, needs to sleep again and gets woken by the OS later again. In your case that definitely happen, since the Mutex is hold for another 50ms. But this is more of an efficiency optimization, not a correctness one.
3. There is no guarantee about startup order of threads.
4. and 5/6: You can only wait on a condition variable while the associated mutex is owned, because waiting will release the mutex for this duration. That is the error you are getting.

Answer 2

As for unlock before notify_one, cppreference says following (emphasis mine):

The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock. However, some implementations (in particular many implementations of pthreads) recognize this situation and avoid this "hurry up and wait" scenario by transferring the waiting thread from the condition variable's queue directly to the queue of the mutex within the notify call, without waking it up.

Notifying while under the lock may nevertheless be necessary when precise scheduling of events is required, e.g. if the waiting thread would exit the program if the condition is satisfied, causing destruction of the notifying thread's condition_variable. A spurious wakeup after mutex unlock but before notify would result in notify called on a destroyed object.

More detailed it is explained here.