Multithread to print odd and even numbers

Question

I recently started on C++ multithreading. Here is my code to print odd and even numbers in two different thread. Can somebody please review this code?

#include <iostream>
#include <thread>
#include <mutex>
#include <condition_variable>
using namespace std;
int x = 1;
mutex m;
bool evenready = false;
bool oddready = true;
condition_variable cond;
void printEven()
{
 for (; x < 10;) {
 unique_lock<mutex> mlock(m);
 cond.wait(mlock, [] {
 return evenready;
 });
 oddready = true;
 evenready = false;
 cout << "Even Print" << x << endl;
 x++;
 cond.notify_all();
 }
}
void printOdd()
{
 for (; x < 10;){
 unique_lock<mutex> mlock(m);
 cond.wait(mlock, [] {
 return oddready;
 });
 oddready = false;
 evenready = true;
 cout << "Odd Print" << x << endl;
 x++;
 cond.notify_all();
}
}
int main()
{
 thread t1(printOdd);
 thread t2(printEven);
 t1.join();
 t2.join();
 return 0;
}

Answer 1

I see some things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Eliminate global variables where practical

The code declares and uses 5 global variables. Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them. That way the linkage is explicit and may be altered more easily if needed. For example, one way to do it would be to gather all of the variables into a struct and pass a reference to the struct to each thread instance. The structure instance could be a local variable within main.

Think about eliminating redundant variables

Since boolean variables evenready and oddready are always in opposite states, one of them is redundant. In fact, in this case, both are redundant since one can easily derive the same function from the value of x.

Use appropriate C++ idioms

This line is somewhat strange:

for (; x < 10;) {

It's much more clear to write like this:

while (x < 10) {

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Don't Repeat Yourself (DRY)

If you're writing almost identical functions, think if there's a way to consolidate them. In this case there certainly is, as I'll demonstrate later in this answer.

Think carefully about data race conditions

A mutex is generally used to assure non-conflicting access to a shared resource. For that reason, it's good to clearly answer the question, "what shared resource is this mutex protecting?" In this case, it seems to be protecting access to std::cout and x but it doesn't do a thorough job of that. Consider that when one thread is evaluating x < 10 (without a lock) the other might be incrementing x (with a lock). That's a classic data race. Here's a rewrite that avoids this problem:

#include <iostream>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <functional>
#include <string_view>
struct OddEven {
 int x = 1;
 std::mutex m;
 std::condition_variable cond;
};
void printTask(OddEven &oe, const std::string_view &label, bool odd)
{
 for (bool running{true}; running; ) {
 std::unique_lock<std::mutex> mlock(oe.m);
 oe.cond.wait(mlock, [&oe, odd] {
 return (oe.x & 1) == odd;
 });
 std::cout << label << oe.x << std::endl;
 oe.x++;
 running = oe.x < 10;
 oe.cond.notify_all();
 }
}
int main()
{
 OddEven oe;
 std::thread t1(printTask, std::ref(oe), "Odd Print", true);
 std::thread t2(printTask, std::ref(oe), "Even Print", false);
 t1.join();
 t2.join();
}

• Deduplicator: Passing a std::string_view by constant reference? Now I've seen everything.

Answer 2

Multiple threads are usually used to compute things in parallel. In this example nothing is computed in parallel: while one thread is running, the other is waiting. With no practical value, it's not a great demonstration of multithreading. I suggest to look for more practical targets in the future.

Since evenready and oddready always have opposite values, one of them would be enough, and less error-prone.

Instead of for (; x < 10;) { it's more natural to use while (x < 10) {.

using namespace std; is considered bad practice.

• samnaction: i used two bools so that odd thread always starts first
• janos: @samnaction you can achieve the same using a single bool: evenready and !evenready, or oddready and !oddready
• user175869: The for loops make sense, but the x++ is in a rather strange place.

Answer 3

I agree with @janos that threads are a poor fit for this problem.

Modularity

If you're going to use threads anyway, I'd at least attempt to get rid of the global variables, and the dependencies between the two threads. Each thread should basically do its "thing" in isolation from the others. As it is right now, your printEven basically knows about and depends on some of the internals of printOdd and vice versa--i.e., that the other will increment x each time it executes. I'd prefer that each be at least reasonably independent, something on this general order:

#include <iostream>
#include <thread>
#include <atomic>
int main() {
 static const int max = 10;
 enum which { odd, even };
 std::atomic<which> t = odd;
 auto a = std::thread([&] {
 for (int i = 1; i < max; i += 2) {
 while (t != odd)
 std::this_thread::yield();
 std::cout << i << "\t";
 t = even;
 }
 });
 auto b = std::thread(
 [&] {
 for (int i = 2; i < max; i += 2) {
 while (t != even)
 std::this_thread::yield();
 std::cout << i << "\t";
 t = odd;
 }
 });
 a.join();
 b.join();
}

Ideally, we'd probably prefer to avoid the knowledge of other threads implicit in odd setting t to even, and even setting t to odd, but each just doing something like t = successor(t);, so deciding what happens next after an iteration of a thread is somewhere else (but it's arguable whether it's worth the trouble for a case this trivial).

Avoid std::endl

Although you're producing little enough output in this case that it probably doesn't much, I'd advise avoiding std::endl in general. Most of the time you just want a new-line, in which case '\n' works fine. If you really do want to flush the stream (like std::endl does) it's better to do that explicitly.

condition variables

A condition variable seems to compound the problem of having threads that do tiny bits of work and constantly switch contexts. The code above (using an atomic and yield()) is simpler (at least IMO) and at least in a quick test, seems to run around three times as fast (and yes, for that test, I modified them to produce the same output, eliminate std::endl as advised above, and piped the output to a file, so I could time this code, not just the speed of console scrolling).