CODEWITHSUNDEEP
c++multithreadingc++11
tree
tree

Reputation: 239

Thread safe wrapper around std::queue doesn't build if storing unique_ptr, but std::queue works

I have a supposed thread safe blocking queue implementation which is supposed to be a wrapper around std::queue Below is its implementation:

template <typename _Tp>
class SharedQueue
{
  public:
    explicit SharedQueue(bool isBlocking = true) : isBlocking_(isBlocking) {}

    virtual ~SharedQueue() {}

    virtual const _Tp& front()
    {
        std::unique_lock<std::mutex> mlock(mtx_);

        // if this is a blocking queue, wait to be notified when when a new object is added
        if (isBlocking_)
        {
            while (queue_.empty())
            {
                cv_.wait(mlock);
            }
        }

        return queue_.front();
    }

    virtual bool empty() const
    {
        std::unique_lock<std::mutex> mlock(mtx_);

        return queue_.empty();
    }

    virtual size_t size() const
    {
        std::unique_lock<std::mutex> mlock(mtx_);

        return queue_.size();
    }

    virtual void push(const _Tp& value)
    {
        std::unique_lock<std::mutex> mlock(mtx_);

        queue_.push(value);

        if (isBlocking_)
        {
            if (queue_.size() == 1)
            {
                cv_.notify_all();
            }
        }
    }

    virtual void push(_Tp&& value)
    {
        {
            std::unique_lock<std::mutex> mlock(mtx_);

            queue_.push(std::move(value));

            if (isBlocking_)
            {
                if (queue_.size() == 1)
                {
                    cv_.notify_all();
                }
            }
        }
    }

    template <typename... _Args>
    void emplace(_Args&&... __args)
    {
        {
            std::unique_lock<std::mutex> mlock(mtx_);

            queue_.emplace(std::forward<_Args>(__args)...);

            if (isBlocking_)
            {
                if (queue_.size() == 1)
                {
                    cv_.notify_all();
                }
            }
        }
    }

    virtual void pop()
    {
        std::unique_lock<std::mutex> mlock(mtx_);

        if (!queue_.empty())
        {
            queue_.pop();
        }
    }

  private:
    bool isBlocking_;

    mutable std::mutex mtx_;

    mutable std::condition_variable cv_;

    std::queue<_Tp> queue_;
};

I want to be be able to place unique_ptr's on this queue and I understand that I'd have to call std::move on the unique_ptr when pushing it on the queue from a client application. Here's my problem... In my main, when I create a std::queue as follows, my program compiles just fine

std::queue<std::unique_ptr<int32_t>> q1;

However, when I create an instance of my SharedQueue as below, the program does not compile.

SharedQueue<std::unique_ptr<int32_t>> q2;

I get an error telling me the copy constructor was deleted in the unique_ptr class, which is understandable. I guess I'm just wondering what std::queue does that the code can compile while my code 'appears' to be a wrapper around the std::queue and implements the operations similarly.

EDIT: I can get the SharedQueue to compile if I replace

queue_.push(value);

with 

queue_.push(std::forward<_Tp>(const_cast<_Tp&>(value)));

Inside the push method that takes in an lvalue reference.

Upvotes: 0

Views: 337

Answers (1)

Joel Filho
Joel Filho

Reputation: 1300

Your problem relies in:

    virtual void push(const _Tp& value)
    {
        std::unique_lock<std::mutex> mlock(mtx_);
        queue_.push(value);
     [...]

If you remove the virtual keyword, it'll compile fine, and it'll fall back to the std::queue case: It'll only emit an error if you use the function.

I assume that using virtual emits the error because, since it requires the compiler to generate a vtable, the function code needs to be generated and pointed to.

A way to keep it virtual and still work would probably be by using SFINAE with std::is_copy_constructible and std::is_move_constructible specializations of a class inheriting from your base, where the push member function is pure virtual.

Here's a (Bad. Don't do OO C++ like that. Really) sample implementation of that idea. Notice that, the same way you can't just "not call in code" the virtual push, you can't just delete the push on the specialization and must do something on runtime. In this example, an exception is thrown.

There are better ways to do that. I'm considering you really need those member functions to be virtual. Otherwise, your problem would be solved by just removing the keyword.

One would be making full specializations of your SharedQueue class, putting the push overloads where you need them, but reusing the rest of the code in composition way, instead of by inheritance.

Another one would be making the same thing on a queue wrapper, which would be smaller and more readable, then using it in your current code (Example, again, not good, but illustrates the point).

The elegant, C++17-or-newer way, is just using if constexpr: 

virtual void push(const _Tp& value) {
    std::unique_lock<std::mutex> mlock(mtx_);

    if constexpr(std::is_copy_constructible_v<_Tp>){
      queue_.push(value);
    } else{
      throw std::invalid_argument("The type _Tp can't be copy constructed");
    }

    if (isBlocking_) {
      if (queue_.size() == 1) {
        cv_.notify_all();
      }
    }
  }

Upvotes: 3

Related Questions