0

I'm trying to pass objects by value between threads with a queue.

Because there are multiple different objects, I made an abstract class.

class message
{
public :
      virtual ~message(){};
};

then I have a subclass for each different type of message

class a_specific_message : public message
{
...
};

I read this tutorial for implementing the queue and I call it the following way:

concurrent_queue<message> queue;
a_specific_message m{1, 2, 3};
queue.push(m);

My problem is that I need to override the operator= so the queue can clone the message

popped_value=the_queue.front();

I tried to add a virtual operator but it doesn't get called in the subclass.

I don't know how I could achieve something like this without passing object by reference.

Guillaume Racicot
  • 39,621
  • 9
  • 77
  • 141
Marc
  • 16,170
  • 20
  • 76
  • 119
  • 1
    `concurrent_queue` holds **objects** of type `message`. When you push an object of a derived type into the queue, the object gets sliced and all that's stored is the `message` part of it. There's no way to get the original object back once that's happened. You need a queue that holds references or pointers. – Pete Becker Dec 07 '16 at 18:43
  • @PeteBecker I was scared that this would happen. How do I make sure that the other thread delete the object when it's done? – Marc Dec 07 '16 at 18:44
  • 2
    Sounds like a job for `shared_ptr` (which is thread safe) – Donnie Dec 07 '16 at 18:45
  • @Marc create virtual destructor – W.F. Dec 07 '16 at 18:45
  • @W.F. Still gets sliced. Needs a pointer or reference. – user4581301 Dec 07 '16 at 18:46
  • @Marc In my opinion, virtual functions and inheritance is not the answer. You should have different queue for different types of messages, and only access the queue you need for the message types you need. – Guillaume Racicot Dec 07 '16 at 18:47
  • @Donnie Might want to elborate that a little bit. Construction and destruction is thread safe, access is not. – NathanOliver Dec 07 '16 at 18:47
  • @GuillaumeRacicot You can invite extra synchronization problems with multiple queues. I usually waste space here and make one generic intertask message and switch interpretations on an enum message type. Mind you I'm also coming in from C and this may be a really stupid thing to do in C++. – user4581301 Dec 07 '16 at 18:52
  • @Donnie I didn't want to use pointers because devs will have to remember to delete it in another thread. It could be easy to forget and have a memory leak. Shared_ptr won't fix that – Marc Dec 07 '16 at 18:57
  • @Marc `std::shared_ptr` is a ref counting smart pointer, and take care of deleting the pointer when the count goes down to `0`. You don't need to manually delete it and it doesn't leak memory. – Guillaume Racicot Dec 07 '16 at 18:58
  • @GuillaumeRacicot Wow this is fantastic! It's amazing. Thank you – Marc Dec 07 '16 at 20:03
  • @Marc you should take a look at my answer, it shows how to do it with unique pointers, which are much more lightweight. – Guillaume Racicot Dec 07 '16 at 20:29

2 Answers2

2

As @PeteBecker writes in the comments, concurrent_queue<message> holds message's by value. When a derived object instance is pushed into the queue it makes a copy of message part only. Slicing occurs.

One way to make the queue hold objects of multiple types without resorting to pointers is to use a discriminated union, e.g. boost::variant:

using message = boost::variant<specific_message_a, specific_message_b>;

No common base class for messages is required here.

The downside of this approach is that sizeof(message) is sizeof of the largest type in that boost::variant<> template argument list.

Maxim Egorushkin
  • 131,725
  • 17
  • 180
  • 271
2

In my opinion, polymorphism is not what you really want. An interface with nothing but a destructor and dynamic downcast usually means that you are using the wrong tool.

However, if polymorphism is what you want, let me propose std::unique_ptr. You can have here you queue of pointers declared like that: concurrent_queue<std::unique_ptr<message>>

As you told in the comments:

I didn't want to use pointers because devs will have to remember to delete it in another thread. It could be easy to forget and have a memory leak.

Then I think std::unqiue_ptr is the right thing for you.

If I translate your code, it would look like that:

concurrent_queue<std::unique_ptr<message>> queue;
auto m = std::make_unique<a_specific_message>(1, 2, 3);

queue.push(std::move(m));

Or simply:

concurrent_queue<std::unique_ptr<message>> queue;

queue.push(std::make_unique<a_specific_message>(1, 2, 3));
// Or
queue.push(new a_specific_message{1, 2, 3});

Then, to pop values:

auto popped_value = std::move(the_queue.front());

Then everything is deleted automatically, without having to remember to delete any pointer. std::unique_ptr has been created for that purpose.

To avoid the explicit move in your code, you could have something like pop_value in your queue, implemented like that:

T pop_value() {
    auto value = std::move(the_queue.front());

    the_queue.pop();

    // use nrvo
    return value;
}

So now, in your threads, you can safely do that:

{
    auto some_message = queue.pop_value();

    // stuff

} // some_message deleted here.
Guillaume Racicot
  • 39,621
  • 9
  • 77
  • 141