4
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Testing and verifying my results is very confusing. My current test-harness creates six logs with a 1:1, 1:10, 1:100, 1:1000 ratio of readers to writers and vice-versa. I specify a thread runtime of 15seconds. However, the results are usually so close together that I'm just clueless as to whether or not this is correct. Can you give me an example of proper test values? More than 1000 threads each? Longer than a minute? \$\endgroup\$ – IAE Dec 21 '11 at 9:52
  • 3
    \$\begingroup\$ Thousands of threads is a bad approach (the more threads the more CPU scheduling). I would use ratios where there is no single reader or writer (like 2:2). Increase thread running time to at least several minutes (the more the better). Ideally your test should validate constantly if there was any state corruption. Run the test for at least couple of hours to be sure that everything is fine. \$\endgroup\$ – Vadym Stetsiak Dec 21 '11 at 10:17

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.