emitting signal causes destructor call

mzimmers

Hi all -

So, I have a program with a worker routine as follows:

void Worker::run()
{
    int len;

    running.ref(); // set to value of 1
    while (running)
    {
        len = sm.recv(buffIn, sizeof(buffIn));
        if (len >= 0)
        {
            buffIn[len] = '\0';
            Message msg(MSG_UNKNOWN, buffIn);
            emit(newMessage(msg));
        }
        Sleep(10);
    }
    emit reachedEndOfThread();
}

Message code is:

Message::Message(MsgType type, char *buffer) : m_type(type)
{
.
.
.
    hash = new(XmlHash);
}

Message::~Message()
{
    delete hash;
}

The emit in Worker::run() causes the ~Message() to be called twice (creating a seg fault on the 2nd call). Any ideas?

Thanks...

Chris Kawa

The Message instance is being copied at the emission point because (I assume) your connection is of queued type.

The way to fix it is to provide a proper copy constructor for the Message class, as the automatically generated one will simply copy the hash pointer and thus make it share the same data among two instances, which will crash on the second one's destructor.

mzimmers

Oh man...what a rookie mistake.

As long as I have your attention, how would you recommend implementing the section of the copy constructor that deals with the unordered_map member? I've gotten as far as:

    XmlHash::iterator it;
    hash = new(XmlHash);
    for (it = from.hash->begin(); it != from.hash->end(); ++it)
    {
        
    }

but I'm unsure how to use the iterator to retrieve the hash key. Once I have the key, I can get the value easily enough (I think).

Thanks...

SGaist

Hi,

Since it's an std::unordered_map, the iterator returns a std::pair containing the key and the value.

mzimmers

Hi SGaist -

Thanks for the refresher on that. I still struggle with the operators for this class.

I think the easiest way to copy the entire table is:

Message::Message(const Message &from)
{
    hash = new(XmlHash);
    hash->insert(from.hash->begin(), from.hash->end());
}

SGaist

Why not use the copy constructor of std::unordered_map ?

mzimmers

If hash were simply an unordered_map, I could just do:

hash = from.hash;

right? But I don't know how to code that when hash is a pointer.

SGaist

hash = *otherHashPointer;

But why is it a pointer in the first place ?

Chris Kawa

As @SGaist pointed out you could make it a non-pointer. This would spare you implementing a copy constructor and a destructor, as the automatically generated ones would do the right thing.

If, however, you want that pointer anyway, then you can just use map's copy constructor:

hash = new XmlHash(*from.hash);

Btw. This is unrelated but I'm just curious - what's with the weird syntax in new(XmlHash) or emit(newMessage(msg))? What do you need those extra () for?

mzimmers

You know, that's a damned good question. It no longer is a pointer. (Of course, now I no longer need the copy constructor, but I'm going to keep it since I struggled so much with it.)

Chris: no good reason for the extra parens...they're probably relics of various attempts at getting this working. They too are gone now.

Thanks, guys.