Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

emitting signal causes destructor call



  • 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...


  • Moderators

    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.



  • 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...


  • Lifetime Qt Champion

    Hi,

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



  • 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());
    }
    

  • Lifetime Qt Champion

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



  • 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.


  • Lifetime Qt Champion

    hash = *otherHashPointer;

    But why is it a pointer in the first place ?


  • Moderators

    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?



  • 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.


Log in to reply