3
\$\begingroup\$

I was asked to write a code in C++ using threads, in which I have to have two limits and 4 characters. Characters can go inside the area limited at the same time when they approach from opposite sides. In addition, a ball moving forward and backwards affects the process in such a way that when the ball is inside the area, no letter can come in, however, if the letter is already in the area it can still move.

My code works, but I got this comment:

"You need to improve the usage of cv.wait() and the definition of the critical area in your code that is guarded with a mutex."

The point is that I have no idea how it can be improve. What is the problem with my cv.wait() function and the area guarded by the mutex?

My code:

#include <thread>
#include <cstring>
#include <unistd.h>
#include <chrono>
#include <ncurses.h>
#include <mutex>
#include <condition_variable>
//Task description:
//First stage: in the first stage I had to have 4 letters which go back and forth with different speeds.
//Second stage: For the second stage I had to have a critical section in which there can be only one letter from each side
//If characters approach from one side the stop, but a character from the other side can proceed. Process occurs independently.
//Third task:
//5th ball flying around back and forth and when the ball will be in the critical area, no ball can enter until that ball leaves while all the other rules are kept,
// if the others are inside they can still move.

using namespace std;
bool ready = true;
mutex m;
mutex m1;
std::condition_variable  cv;
int A=19,B=19,C=0,D=0,O;


void ball(int &point, int speed)
{
    bool incrementing = true;
    while(ready)
    {
        if(incrementing)
        {
            point ++;
            this_thread::sleep_for(chrono::milliseconds(speed));
        }

        if(point == 20 ||  point == 0)// Length of the line is 20
            incrementing = !incrementing;

        if(!incrementing)
        {
            point --;
            this_thread::sleep_for(chrono::milliseconds(speed));
        }
        if((point < 5 || point > 15)){
        cv.notify_all();
        this_thread::sleep_for(chrono::milliseconds(speed));
        }
        }


    }




void worker(int &point, int speed)
{
    bool incrementing = true;
    while(ready)
    {
        if(incrementing)
        {
            point ++;
            this_thread::sleep_for(chrono::milliseconds(speed));
        }

        if(point == 20 ||  point == 0)// Length of the line is 20
            incrementing = !incrementing;

        if(!incrementing)
        {
            point --;
            this_thread::sleep_for(chrono::milliseconds(speed));
        }


        if(point > 5 && point < 15 && incrementing){
        unique_lock<mutex> lock (m);
        cv.wait(lock);
        while(point > 5 && point < 15){
        point ++;
        this_thread::sleep_for(chrono::milliseconds(speed));
        }
        }


        if(point > 5 && point < 15 && !incrementing){
        unique_lock<mutex> locki (m1);
        cv.wait(locki);
        while(point > 5 && point < 15){
        point --;
        this_thread::sleep_for(chrono::milliseconds(speed));
        }
        }
    }
}







void Printing()
{
    while(ready)
    {
        this_thread::sleep_for(chrono::milliseconds(20));
        erase();

        mvaddch(0,A,'A');
        mvaddch(1,B,'B');
        mvaddch(2,C,'C');
        mvaddch(3,D,'D');
        mvaddch(4,O,'O');
        refresh();
        mvprintw(5,0,"------|-------|------");
        //for(int i = 0; i <= 3; i++){
        //mvprintw(i,6,"|    |");

        refresh();
        }


}



int main()
{
    initscr();
    thread thread1{[]{worker(A,500);}};
    thread thread2{[]{worker(B,500);}};
    thread thread3{[]{worker(C,500);}};
    thread thread4{[]{worker(D,500);}};
    thread threadball{[]{ball(O,300);}};
    thread thread5= thread(&Printing);

    if(getch())
    {
        ready = false;
        thread1.join();
        thread2.join();
        thread3.join();
        thread4.join();
        threadball.join();
        thread5.join();
    }
    endwin();
    return 0;
}


\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate - e.g. something like "printing ball position"?. \$\endgroup\$ Jun 15 at 20:14
  • 1
    \$\begingroup\$ I don't see how the approach from the opposite sides is addressed. \$\endgroup\$
    – vnp
    Jun 15 at 20:46
  • 1
    \$\begingroup\$ @vnp From my reading, by the two mutexes m and m1, for left and right approaches respectively, in the two if blocks at the end of worker(). You get in the first block if you’re entering the critical area from the left, then freeze (because of the mutex) if anyone else is already in there (after approaching from the left), until they leave (and release the mutex, and you get it), at which point you enter, loop until you exit the area, and then exit the block (releasing the mutex for the next guy). Basically same goes for the other direction in the following block. \$\endgroup\$
    – indi
    Jun 16 at 2:29

2 Answers 2

2
\$\begingroup\$

Edward has given a very good answer, and I agree with everything he said. However, he didn't address this issue:

“You need to improve the usage of cv.wait() and the definition of the critical area in your code that is guarded with a mutex.”

The point is that I have no idea how it can be improve. What is the problem with my cv.wait() function and the area guarded by the mutex?

While your use of mutexes to guard against more than one character from entering the limited area from a given side might be defensible, the problem is that you cannot use condition variables on their own to signal that something has happened. The reason is that cv.wait() might return even if there was no cv.notify_all(). So you also need some state that you can check, and redo the cv.wait() if necessary. That state must be guarded by the mutex you are passing to cv.wait().

Apart from that, locking mutexes for an extended period is time is usually a bad idea. So I would try to find a solution that does not call std::this_tread::sleep_for() while holding a lock.

Furthermore, the only time something interesting happens is when a ball or character is entering or exiting the limited area; when they are completely inside or outside, you should not have to do anything. Your code lets the ball call cv.notify_all() every step it makes outside the limited area.

I would structure the code so that there is a single mutex that is only ever held for short durations, and only use the condition variable to signal that the ball or character has just left the limited area. For example:

std::mutex m;
std::condition_variable  cv;
bool ball_inside = false;
bool from_left = false;
bool from_right = false;
...
void worker(int &point, std::chrono::milliseconds interval)
{
    bool incrementing = true;

    while(ready) {
        {
            std::lock_guard lk(m);
            int next_point = point + incrementing ? 1 : -1;

            if (incrementing) {
                if (next_point == critical_begin) {
                    cv.wait(lk, [&]{return !from_left && !ball_inside;});
                    from_left = true;
                } else if (point = critical_end) {
                    from_left = false;
                    cv.notify_all();
                }
            } else {
                ...
            }
            ...
            point = next_point;
        }

        std::this_thread::sleep_for(interval);
    }
}

Note that the sleep_for() is done without the lock held, and while we call cv.wait() with the lock held, it will automatically unlock it while it is waiting. The lambda passed to cv.wait() checks for the condition that must be waited upon, so you don't have to implement a loop to restart it on spurious returns yourself.

The above code can still be improved with the techniques Edward pointed out. In particular, find a way to remove code duplication. That will make the code easier to maintain, and will result in less chance of errors. For example, did you notice that in your program, the ball moves twice as fast if it is inside the critical region than it does outside?

\$\endgroup\$
1
  • \$\begingroup\$ Well said! I meant to get back to that review and address this, but you've done it perfectly. \$\endgroup\$
    – Edward
    Jun 18 at 13:02
2
\$\begingroup\$

I see a number of 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. Know when to use it and when not to (as when writing include headers).

Know how to pass things to threads

The current code contains these lines

thread thread4{[]{worker(D,500);}};
thread threadball{[]{ball(O,300);}};
thread thread5= thread(&Printing);

Those aren't necessarily wrong, but it may be useful for you to know the alternatives. Here's another way to write those:

std::thread thread4{worker, std::ref(D), 500};
std::thread threadball{ball, std::ref(O), 300};
std::thread thread5{Printing};

To me, these seem a litle more clear. Note that we need to use std::ref because we don't want to just pass a value to each thread but a reference to a variable. Also for the last line, we can just construct the thread directly and the name of a function does not need further qualification with &.

Fix your formatting

There are inconsistent spaces at the beginning of lines, inconsistent indentation and inconsistent use and placement of curly braces {}. Being consistent helps others read and understand your code.

Try to use descriptive names

I didn't find much use in the names m, m1 or cv. Those names don't convey much meaning to other programmers, or even yourself if you pick up this code six months after you wrote it.

Consider using a collection of threads

As per the previous suggestion, he names of the threads (thread1, thread2, etc.) are not very useful or descriptive. Rather than trying to come up with names, I'd be inclined to leave them as anonymous and put then into a std::vector. Here's one way to do that:

std::vector<std::thread> v;
v.emplace_back(std::thread{worker, std::ref(A) ,500});
v.emplace_back(std::thread{worker, std::ref(B) ,500});
v.emplace_back(std::thread{worker, std::ref(C) ,500});
v.emplace_back(std::thread{worker, std::ref(D), 500});
v.emplace_back(std::thread{ball, std::ref(O), 300});
v.emplace_back(std::thread{Printing});

if(getch())
{
    ready = false;
    for (auto &thr: v)
        thr.join();
}

Eliminate magic numbers

The constants 5, 15 and 20 are used in multiple places. It would be better to have them as named const values so that it would be clear what those numbers represent.

Eliminate redundant code

Instead of repeating much of the code for moving things in worker and ball, there are a number of things that could be factored out to simplify. To begin with, rather than a boolean value incrementing, I'd suggest using an int value increment and simply toggle that back and forth from +1 to -1. Here's one way to do that:

constexpr mytype critical_begin{5};
constexpr mytype critical_end{15};

void at_end(mytype p, mytype &increment) {
    if ((p <= 0 && increment < 0) || (p >= 20 && increment > 0))
        increment *= -1;
}

bool in_critical(mytype p) {
    return p > critical_begin && p < critical_end;
}

void advance(mytype& point, mytype& increment, int speed) { 
    point += increment;
    at_end(point, increment); 
    std::this_thread::sleep_for(std::chrono::milliseconds(speed));
}

void ball(mytype &point, int speed, bool right)
{
    mytype increment{ right ? +1 : -1 };
    while(ready)
    {
        advance(point, increment, speed);
        if(!in_critical(point)) {
            cv.notify_all();
        }
    }
}

Naturally, you'd make the corresponding changes to worker, and setting up the threads would look like this:

v.emplace_back(worker, std::ref(A), 500, false);
v.emplace_back(worker, std::ref(B), 500, false);
v.emplace_back(worker, std::ref(C), 500, true);
// etc.

I had originally written that these would be written like this:

v.emplace_back(std::thread{worker, std::ref(A), 500, false});
v.emplace_back(std::thread{worker, std::ref(B), 500, false});
v.emplace_back(std::thread{worker, std::ref(C), 500, true});

But as @Deduplicator asked in a comment, "why move from a temporary instead of really emplacing?" It makes a real difference, as shown in this live code.

Explain the code clearly

I am not sure that I understand the rules enumerated in the comments of the code. They are not well written. If you received them as instructions, you should seek to clarify them so that they are clear when you repeat them as comments. If you paraphrased your instructions, I'd recommend working on describing things more clearly.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ If you emplace, why move from a temporary instead of really emplacing? \$\endgroup\$ Jun 19 at 21:57
  • \$\begingroup\$ Good question -- an error of cut and paste! I'll update the answer and also show the difference. Thanks! \$\endgroup\$
    – Edward
    Jun 20 at 11:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.