Correct multithreaded reader-writer implementation

Question

I think I'm finished writing a multithreaded reader-writer implementation for my Operating Systems course. I would like to verify that my multithreading is correct and that I'm using good C++0x style.

PalindromeDatabase.h

class PalindromeDatabase : public Utility::Uncopyable
{
    friend void *read(void *classHandle);
    friend void *write(void *classHandle);

public:
    PalindromeDatabase(std::vector<int> const& arguments);

    void process();

private:
    void createReaderThreads();
    void createWriterThreads();

    void joinReaderThreads();
    void joinWriterThreads();

    void *read();
    void readPalindromes();

    void *write();
    void processPalindromes();
    void formatPalindrome(std::string& palindrome);
    void removeNonAlphabeticCharacters(std::string& palindrome);
    void toLowerCase(std::string& palindrome);

private:
    const std::string PalindromeFilename_;

    const int ThreadWorkTime_;

    const int NumberOfReaders_;
    const int NumberOfWriters_;

    std::vector<pthread_t> readerThreads_;
    std::vector<pthread_t> writerThreads_;

    sem_t semaphore_;
    pthread_mutex_t writerMutex_;

    Timer timer_;
};

PalindromeDatabase.cpp

PalindromeDatabase::PalindromeDatabase(std::vector<int> const& arguments)
    : PalindromeFilename_("palindromes"),
      ThreadWorkTime_(arguments[1]),
      NumberOfReaders_(arguments[2]), NumberOfWriters_(arguments[3]),
      readerThreads_(NumberOfReaders_), writerThreads_(NumberOfWriters_)
{
    sem_init(&semaphore_, 0, NumberOfReaders_);
    pthread_mutex_init(&writerMutex_, nullptr);
}

void PalindromeDatabase::process()
{
    Logger().log() << "Starting session...";

    timer_.start();

    createReaderThreads();
    createWriterThreads();

    joinReaderThreads();
    joinWriterThreads();

    timer_.stop();

    Logger().log() << "Ending session...";

    Logger().log() << "Elapsed time:  " << timer_.seconds() << "s:" << timer_.milliseconds() % 1000 << "ms.";

    pthread_exit(nullptr);
}

void PalindromeDatabase::createReaderThreads()
{
    for (auto& thread : readerThreads_)
        pthread_create(&thread, nullptr, ::read, static_cast<void*>(this));
}

void PalindromeDatabase::joinReaderThreads()
{
    for (auto& thread : readerThreads_)
    {
        int garbage = 0;
        pthread_join(thread, reinterpret_cast<void**>(&garbage));
    }
}

void *PalindromeDatabase::read()
{
    timeval startTime;
    gettimeofday(&startTime, nullptr);

    while (Utility::elapsedTime(startTime) < ThreadWorkTime_)
    {
        sem_wait(&semaphore_);

        auto threadID = pthread_self();

        Logger().log() << "Reader " << threadID << " is reading the file...";

        readPalindromes();

        Logger().log() << "Reader " << threadID << " has finished reading the file...";

        sem_post(&semaphore_);

        sleep(1); // Simulate work
    }

    return nullptr;
}

void *PalindromeDatabase::write()
{
    timeval startTime;
    gettimeofday(&startTime, nullptr);

    while (Utility::elapsedTime(startTime) < ThreadWorkTime_)
    {
        pthread_mutex_lock(&writerMutex_);

        for (int i = 0; i < NumberOfReaders_; ++i)
            sem_wait(&semaphore_);

        pthread_mutex_unlock(&writerMutex_);

        auto threadID = pthread_self();
        Logger().log() << "Writer " << threadID << " is working on the file...";

        processPalindromes();

        Logger().log() << "Writer " << threadID << " finished working on the file...";

        for (int i = 0; i < NumberOfReaders_; ++i)
            sem_post(&semaphore_);

        sleep(1); // Simulate work.
    }

    return nullptr;
}

I've kept the code minimal, showing only the multithreaded parts and not the implementation, which is a little nonsensical since we simply had to do "busy work". I've omitted the Logger because I think it's safe and outside of any critical sections.

Answer 1

It is very hard to determine if multi threaded code is correct just by looking at it. You should create special tests that simulate concurrent operations and validate shared state for corruption. The test(s) should be run for considerable amount of time and preferably on multi CPU (core) environment to be sure that code is working correctly.

Here's what I consider suspicious in the code sample you've provided: In PalindromeDatabase::write() you're releasing write mutex soon after you've waited for all readers to complete. If I understand correctly, if there are multiple writers it is possible that they will call processPalindromes() simultaneously.

I propose placing pthread_mutex_unlock(&writerMutex_); after the processPalindromes is called thus write will be thread-safe.