Polymorphic Queue

Question

I'm trying to implement a Polymorphic Queue. Here is my trial:

QQueue <Request *> requests;

while(...)
    {
        QString line = QString::fromUtf8(client->readLine()).trimmed();

        if(...)){                      
            Request *request=new Request();
            request->tcpMessage=line.toUtf8();
            request->decodeFromTcpMessage(); //this initialize variables in request using tcpMessage
            if(request->requestType==REQUEST_LOGIN){
                LoginRequest loginRequest;
                request=&loginRequest;
                request->tcpMessage=line.toUtf8();
                request->decodeFromTcpMessage();
                requests.enqueue(request);
            }
            //Here pointers in "requests" do not point to objects I created above, and I noticed that their destructors are also called.
            LoginRequest *loginRequest2=dynamic_cast<LoginRequest *>(requests.dequeue());   
            loginRequest2->decodeFromTcpMessage();
        }
    }

Unfortunately, I could not manage to make work Polymorphic Queue with this code because of the reason I mentioned in second comment.I guess, I need to use smart-pointers, but how? I'm open to any improvement of my code or a new implementation of polymorphic queue.

Thanks.

This is wrong in so many ways I don't even know where to start. For starters: Why are you first converting the input from UTF-8, and then back to UTF-8? Why isn't `decodeFromTcpMessage()` a free function taking a string and returning a dynamically allocated request? You put enqueue the address of a _local automatic object_ into the queue. (Ouch!) You always try to retrieve a `LoginRequest`, although you also put others into the Queue. You access the result of the `dynamic_cast` without checking whether the casts succeeded. (Why are you casting anyway? What's wrong with virtual functions?) ... — sbi, Apr 08 '10 at 12:28
...I ran out of space, since a comment only allows 600 characters. I suggest you get yourself a decent C++ book. For example, see here: http://stackoverflow.com/questions/388242/. — sbi, Apr 08 '10 at 12:32

score 3 · Accepted Answer · answered Apr 08 '10 at 10:04

There are 2 problems in your source:

you claim memory by the Request *request=new Request();, which gets abandoned by the later request=&loginRequest; assignment (and is no longer deletable)
the LoginRequest loginRequest; variable gets destructed when the execution leaves the {} block where the variable is defined, resulting in a dangling pointer in request

I would suggest to remove the Request *request=new Request(); line, and later in the if(...){ block assign the concrete LoginRequest object by

LoginRequest* loginRequest = new LoginRequest();
/* fill the request */
requests.enqueue(loginRequest);

You can get rid of the queued objects by deleting them manually when they got poped out of the queue (after they are processed), or by using a container-safe smartpointer in the queue (boost::shared_ptr is fine, maybe QT has also one of them, std::auto_ptr IS NOT container-safe).

PITFALL Also make sure that the destructor of Request is virtual, since you cannot delete objects by a pointer to its base classe when there is no virtual destructor in the base class(c++ can call the base class destructor with the derived class instance in this case, leading in undefined behavior like memory leaks or crashes)

pajton · Answer 2 · 2010-04-08T10:03:40.083

You are putting invalid pointer to your QQueue. If your QQueue holds pointers, you need to create every object you insert to it on the heap, i.e. with a call to new. Also, do not forget to free the first created Request if you do not need it.

I think you should rewrite your code to:

...
if(request->requestType==REQUEST_LOGIN){
    delete request;
    request = new LoginRequest();
    request->tcpMessage=line.toUtf8();
    ...
}

With this code, your later dynamic_cast<LoginRequest*> will not fail.

Oleg Zhylin · Answer 3 · 2010-04-08T10:12:07.107

1

Immediately from the code snippet I can see that an object of Request is queued and later you try to downcast it to LoginRequest. dynamic_cast will rightfully fail. You have to parse request data and create objects of appropriate class, derived from Request. I would suggest using Factory Pattern for this.

edited Apr 08 '10 at 10:12

answered Apr 08 '10 at 09:34

Oleg Zhylin

1,290
12
18

score 1 · Answer 4 · answered Apr 09 '10 at 01:51

This is also a good place to use factories, IMO.

if(...)){
   Request *request = CreateRequest(message);
   requests.enqueue(request);
}

Request* request = requests.pop();
LoginRequest* login_req = dynamic_cast<LoginRequest*>(request);
if (login_req != NULL)
   whatever;

where

Request* CreateRequest(TcpMessage* message)
{
   TcpMessage* message = line.toUtf8();
   RequestType type = message->GetRequestType();
   Request* req = NULL;

   switch (type)
   {
   case REQUEST_LOGIN:
      req = new LoginRequest(message);
      break;
   etc.
   }

   delete message;
   return req;
}

...and then, naturally, your constructors do the right thing with the message, initializing the objects properly.

Polymorphic Queue

4 Answers4