two threads entering the critical section on the mutex

Question

The following simplified example of several I'm writing a c++20 software which explits pthreads. The simplified example shows how I have a shared resource shared_resource, an int variable, which is written by several threads, several times. To access the variable I use a mutex and a condition variable. A typical use of mutex and condition variables.

the num_readers is used as following:

• greater than 0: multiple readers accessing the shared variable
• 0: neither writers nor readers are accessing the resource
• -1: a writer is writing a new value on the resource. No more readers nor writers are avaibale until the writer releases the resource

The simplified version has no readers for focusing on the problem. Since num_readers = num_readers - 1; can be executed only when a writer releases the resource by setting it to 0 and signaling the other writers, I expect 0 or -1 values, but never -2!

The problem is that by executing the following I randomly get -2 values, so some interleaving problem is occurring I guess:

WAT>? num_readers -2

Process finished with exit code 1

#include <iostream>
#include <pthread.h>
#include <cstdlib>
#include <thread>
#include <random>

void* writer(void* parameters);

pthread_mutex_t mutex{PTHREAD_MUTEX_DEFAULT};
pthread_cond_t cond_writer = PTHREAD_COND_INITIALIZER;

int num_readers{0};
int shared_resource{0};

int main() {
    const int WRITERS{500};

    pthread_t writers[WRITERS];

    for(unsigned int i=0; i < WRITERS; i++) {
        pthread_create(&writers[i], NULL, writer, NULL);
    }

    for(auto &writer_thread : writers) {
        pthread_join(writer_thread, NULL);
        std::cout << "[main] writer returned\n";
    }

    std::cout << "[main] exiting..." << std::endl;
    return 0;
}

void* writer(void* parameters) {
    for (int i=0; i<5; i++) {
        pthread_mutex_lock(&mutex);

        while(num_readers != 0) {
            if (num_readers < -1) {
                std::cout << "WAT>? num_readers " << std::to_string(num_readers) << "\n";
                exit(1);
            }

            pthread_cond_wait(&cond_writer, &mutex);
        }

        num_readers = num_readers - 1;

        pthread_mutex_unlock(&mutex);

        std::uniform_int_distribution<int> dist(1, 1000);
        std::random_device rd;
        int new_value = dist(rd);

        shared_resource = new_value;

        pthread_mutex_lock(&mutex);

        num_readers = 0;

        pthread_mutex_unlock(&mutex);

        pthread_cond_signal(&cond_writer);
    }


    return 0;
}

So: why isn't this code thread safe?

Answer 1

Some issues stand out in your code:

1. You modify the number of readers in the write funtion. Only the reader function should do that.

2. Same thing for the signaling of the condition variable. That should only be signaled from the reader function.

3. incrementing and decrementing the number of readers is usually done with a semaphore: an atomic int and an associated condition variable.

Here is the algorithm:

int reader()
{
    // indicate that a read is in progress.  
    //
    //  a. lock()/
    //  b. increment number of readers.
    //  c. unlock() as soon as possible, so other readers can also read reading.
    //
    // note that any write in progress will stop the thread here.
    pthread_mutex_lock(&mutex);
    ++num_readers;
    pthread_mutex_unlock(&mutex);

    // read protected data
    int result = shared_resource;

    // decremennt readers count.
    // 
    // note that calls to lock()/unlock() are not necessary if 
    // num_readers is atomic (I.e.: std::atomic<int>)
    pthread_mutex_lock(&mutex);
    if (--num_readers == 0)
        pthread_cond_signal(&cond_writer);  // last reader sets the cond_var 
    pthread_mutex_unlock(&mutex);

    return result;
}

void writer(int value)
{
    // lock
    pthread_mutex_lock(&mutex);

    // wait for no readers, the mutex is released while waiting for
    // the last read to complete.  Note that access to num_readers is 
    // done while the mutex is owned.
    while (num_readers != 0)
        pthread_cond_wait(&cond_writer, &mutex);
 
   // modify protected data.
   shared_resource = value;

   // unlock.
   pthread_mutex_unlock(&mutex);
}