CODEWITHSUNDEEP
c++queuec++17
noah
noah

Reputation: 107

queue locking up with multiple threads

I am trying to implement a thread safe queue using the include in c++17. Few notes: uint8_t is purposely used instead of a boolean. MultiQueue is just a template because I need to hold 3 different types within my program so instead of writing 3 different functions for each type i used a template. All of this is stored in a file called queue_safe.h
Problem: The queue class seems to be locking up and doing absolutely nothing and timing out THEN instead of returning it executes the code under this comment /* if a timeout was not hit, but the queue was empty previously this will execute */. This results in a cant call .front() on an empty queue.
Things to consider: The queue is definitely not empty, as before launching the threads i can call .get() and something will be "couted" to the screen.
What works: single threaded works fine. It will wait 20 seconds if the queue is empty, then it will return.

#pragma once

#include <string>
#include <mutex>
#include <queue>
#include <condition_variable>
#include <chrono>
#include <iostream>

template <typename StoredType>
class SafeQueue {
public:
    std::condition_variable ready;
    std::mutex queue_lock;

    std::queue<StoredType> safe_queue = {};

    uint8_t empty();
    void put(StoredType& element);
    StoredType get();
};


/* must have it in the header file so linker can see it */

template <typename StoredType>
StoredType SafeQueue<StoredType>::get() {

    std::unique_lock<std::mutex> condition_lock(queue_lock);

    if (safe_queue.empty()) {


        if (ready.wait_for(condition_lock, std::chrono::seconds(20)) == std::cv_status::timeout) {

            /* timeout was reached, no items left */

            return StoredType();
        }

        /* if a timeout was not hit, but the queue was empty previously this will execute */

        StoredType element = safe_queue.front();

        safe_queue.pop();

        return element;


    }

    else {

        /* not empty, return an element */

        StoredType element = safe_queue.front();

        safe_queue.pop();

        return element;
    }

    /* do not need a return value here since empty HAS to be either true/false */

}

template <typename StoredType>
void SafeQueue<StoredType>::put(StoredType& element) {

    std::lock_guard<std::mutex> put_guard(queue_lock);

    safe_queue.push(element);

    ready.notify_one();

}

template <typename StoredType>
uint8_t SafeQueue<StoredType>::empty() {

    std::lock_guard<std::mutex> empty_guard(queue_lock);

    if (safe_queue.size() == 0) {

        return 1;
    }

    return 0;

}

Upvotes: 0

Views: 206

Answers (1)

jignatius
jignatius

Reputation: 6494

You probably have a deadlock happening in the put method of SafeQueue<StoredType>.

It's standard practice to allow the lock_guard to release the mutex before calling condition_variable::notify_one. Just add a scope level to the lock/push operations:

template <typename StoredType>
void SafeQueue<StoredType>::put(StoredType& element) {
    {
        std::lock_guard<std::mutex> put_guard(queue_lock);
        safe_queue.push(element);
    }
    ready.notify_one();
}

Doing this avoids a situation where the notified thread immediately locks again.

Do you need to wait 20s in get? That's rather a long timeout. You might want to reduce that to avoid threads waiting a while. You can also simplify the get function to include a predicate in the wait_for call:

template <typename StoredType>
StoredType SafeQueue<StoredType>::get() {

    std::unique_lock<std::mutex> condition_lock(queue_lock);
    ready.wait_for(condition_lock, std::chrono::seconds(20), [&] { return !safe_queue.empty();  });

    if (safe_queue.empty()) {

        return StoredType();
    }
   
    /* not empty, return an element */

    StoredType element = safe_queue.front();

    safe_queue.pop();

    return element;    
}

This has the advantage of protecting against spurious wake-ups with a predicate. Now the wait will only return if the queue is not empty or the timeout occurs.

Upvotes: 0

Related Questions