How to implement semaphore? Is this implementation correct or faulty?

Question

I want to implement semaphore class. And a user on stackoverflow has noted that my implementation is not working correct.

At the first I done it like this:

class sem_t {
    int count;
public:
    sem_t(int _count = 0) : count(_count) {};
    void up() {
        this->count++;
    }
    void down() {
        while (this->count == 0)
            std::this_thread::yield();
        this->count--;
    }
};

Then a user on stackoverflow noted that this implementation is faulty cause I read and write variable count out of any synchronization primitive and at some point the value can become incorrect and in case of compiler optimization compiler can assume that the variable count can not be modified by another thread. So, I tried to add mutex to this construction and I've done it like this:

class sem_t {
    int count;
    std::mutex mutualExclusion;
public:
    sem_t(int _count = 0) : count(_count) {};
    void up() {
        this->mutualExclusion.lock();
        this->count++;
        this->mutualExclusion.unlock();
    }
    void down() {
                this->mutualExclusion.lock();
        while (this->count == 0)
            std::this_thread::yield();
        this->count--;
        this->mutualExclusion.unlock();
    }
};

But in case of using this approach when I try to detach thread i got an error saying that mutex has been destroyed while busy, cause one thread can hold mutex and then yield after which the thread is detached and error occurs (Is this solution ok?). Then I tried to modify this code and I stoped on following construction:

class sem_t {
    int count;
    std::mutex mutualExclusion;
public:
    sem_t(int _count = 0) : count(_count) {};
    void up() {
        this->mutualExclusion.lock();
        this->count++;
        this->mutualExclusion.unlock();
    }
    void down() {
        while (this->count == 0)
            std::this_thread::yield();
        this->mutualExclusion.lock();
        this->count--;
        this->mutualExclusion.unlock();
    }
};

But I think that this solution is faulty too, cause it can lead the same problem as the first solution.

So, whats the correct implementation? I wanna note that I tried implementation with condition variable, but i am trying to implement semaphore without condition variable and if you want to suggest some solution with condition variable please describe how wait method of condition variable is working.

[Edit]

My full code, using self implemented semaphore:

#include "pch.h"
#include <iostream>
#include <vector>
#include <mutex>
#include <thread>
#include <chrono>

class sem_t {
    int count;
    std::mutex mutualExc;
public:
    sem_t(int _count = 0) : count(_count) {};
    void up() {
        mutualExc.lock();
        this->count++;
        mutualExc.unlock();
    }
    void down() {
        mutualExc.lock();
        while (this->count == 0) {
            mutualExc.unlock();
            std::this_thread::yield();
            mutualExc.lock();
        }
        this->count--;
        mutualExc.unlock();
    }
};

#define N 5
#define THINKING 0
#define HUNGRY 1
#define EATING 2

std::mutex mx;
std::mutex coutMX;
char philosopherState[N] = { THINKING };
sem_t philosopherSemaphores[N] = { 0 };

void testSetState(short i) {
    if (philosopherState[i] == HUNGRY && philosopherState[(i + 1) % N] != EATING && philosopherState[(i + N - 1) % N] != EATING) {
        philosopherState[i] = EATING;
        philosopherSemaphores[i].up();
    }
}

void take_forks(short i) {
    ::mx.lock();
    philosopherState[i] = HUNGRY;
    
    testSetState(i);
    ::mx.unlock();
    philosopherSemaphores[i].down();
}
void put_forks(short i) {
    ::mx.lock();
    philosopherState[i] = THINKING;

    testSetState((i + 1) % N);
    testSetState((i + N - 1) % N);
    ::mx.unlock();
}

void think(short p) {
    for (short i = 0; i < 5; i++) {
        coutMX.lock();
        std::cout << "Philosopher N" << p << " is thinking!" << std::endl;
        coutMX.unlock();
        std::this_thread::sleep_for(std::chrono::milliseconds(500));
    }
}
void eat(short p) {
    for (short i = 0; i < 5; i++) {
        coutMX.lock();
        std::cout << "Philosopher N" << p << " is eating!" << std::endl;
        coutMX.unlock();
        std::this_thread::sleep_for(std::chrono::milliseconds(500));
    }
}

void philosopher(short i) {
    while (1) {
        think(i);
        take_forks(i);
        eat(i);
        put_forks(i);
    }
}

int main()
{
    std::vector<std::thread*> threadsVector;
    for (int i = 0; i < N; i++) {
        threadsVector.push_back(new std::thread([i]() { philosopher(i); }));
    }

    std::this_thread::sleep_for(std::chrono::milliseconds(15000));

    for (int i = 0; i < N; i++) {
        threadsVector[i]->detach();
    }

    return 0;
}

Error that is occurring (only occurs when running program in release or debug mode in visual studio)

Answer 1

The last attempt is indeed not correct because it may happend that several threads call down at the same time and all successfully pass

    while (this->count == 0)
        std::this_thread::yield();

lines and then they will all decrement the counter to possiby negative value:

    this->mutualExclusion.lock();
    this->count--;
    this->mutualExclusion.unlock();

So, the counter value check and update must be performed atomically.

If you want to keep busy loop the easiest way would be just to call unlock before yield and lock after, so compare and decrement will be performed under the same lock:

void down() {
    this->mutualExclusion.lock();
    while (this->count == 0) {
        this->mutualExclusion.unlock();
        std::this_thread::yield();
        this->mutualExclusion.lock();
    }
    this->count--;
    this->mutualExclusion.unlock();
}

Also, you can use std::unique_lock guard which locks provided mutex in constructor and unlocks in destructor, so the mutex will not be accidentaly left in locked state:

void down() {
    std::unique_lock<std::mutex> lock(this->mutualExclusion);
    while (this->count == 0) {
        lock.unlock();
        std::this_thread::yield();
        lock.lock();
    }
    this->count--;
}

To deal with "muxed destroyed while busy" error you need either to have a flag to stop background threads and wait for them to complete with join instead of detach:

#include <atomic>

...

std::atomic<bool> stopped{ false };

void philosopher(short i) {
    while (!stopped) {
        ...
    }
}

...

int main()
{
    ...

    stopped = true;
    for (int i = 0; i < N; i++) {
        threadsVector[i]->join();
    }
    return 0;
}

or if you really don't want to care of releasing static resources you can call std::quick_exit instead of detach and return:

int main()
{
    ...
    std::this_thread::sleep_for(std::chrono::milliseconds(15000));
    std::quick_exit(0);
}