Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • Users
  • Groups
  • Search
  • Get Qt Extensions
  • Unsolved
Collapse
Brand Logo
  1. Home
  2. Qt Development
  3. General and Desktop
  4. crash desallocating local string when closing socket

crash desallocating local string when closing socket

Scheduled Pinned Locked Moved Solved General and Desktop
17 Posts 2 Posters 4.4k Views
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • M Offline
    M Offline
    mbruel
    wrote on 4 Jan 2016, 21:06 last edited by
    #8

    Thanks, I thought I tried and it didn't work but probably I queued the wrong connection.
    I'll take this option to queue the disconnect call of the socket. I'll keep the delete of the socket as I want to make sure the pointer is then set to NULL.

    K 1 Reply Last reply 4 Jan 2016, 21:13
    0
    • M mbruel
      4 Jan 2016, 21:06

      Thanks, I thought I tried and it didn't work but probably I queued the wrong connection.
      I'll take this option to queue the disconnect call of the socket. I'll keep the delete of the socket as I want to make sure the pointer is then set to NULL.

      K Offline
      K Offline
      kshegunov
      Moderators
      wrote on 4 Jan 2016, 21:13 last edited by
      #9

      @mbruel
      If you want to be notified of the socket deletion and still wish to use deleteLater you could hold the pointer not as QTcpSocket *, but as a QPointer<QTcpSocket> instead. In that case when the socket is deleted by Qt (through processing of the events) the QPointer instance will be set to point to NULL automatically.

      Kind regards.

      Read and abide by the Qt Code of Conduct

      M 1 Reply Last reply 5 Jan 2016, 11:23
      0
      • K kshegunov
        4 Jan 2016, 21:13

        @mbruel
        If you want to be notified of the socket deletion and still wish to use deleteLater you could hold the pointer not as QTcpSocket *, but as a QPointer<QTcpSocket> instead. In that case when the socket is deleted by Qt (through processing of the events) the QPointer instance will be set to point to NULL automatically.

        Kind regards.

        M Offline
        M Offline
        mbruel
        wrote on 5 Jan 2016, 11:23 last edited by
        #10

        @kshegunov
        I'm trying QPointers and I'm having a compilation issue with the one of my QTextStream. I don't understand what is the problem.
        Here is what I did:

        definition:
            QPointer<QTcpSocket>   iSocket;
            QPointer<QTextStream>  iStream;
        
        void MonitorServer::newConnection(){
            QTcpSocket *sock = iServer->nextPendingConnection();
            if (iSocket.isNull()){
                _log("Ignore Connection: Monitor Server has already a local connection...");
                sock->write("Already a connection in use...\n");
                sock->flush();
                delete sock;
            } else {
                iSocket = sock;
                iStream = new QTextStream(sock);
        
                connect(this, &MonitorServer::destroyMonitorConnection, iSocket.data(), &QObject::deleteLater);
                connect(this, &MonitorServer::destroyMonitorConnection, iStream.data(), &QObject::deleteLater);
        
        }
        

        I'm getting this issue:

        monitorserver.cpp:62: error: no matching function for call to 'MonitorServer::connect(MonitorServer*, void (MonitorServer::*)(), QTextStream*, void (QObject::*)())'
                 connect(this, &MonitorServer::destroyMonitorConnection, iStream.data(), &QObject::deleteLater);
                                                                                                              ^
        

        It must be silly but I don't see why I'm getting this issue with the QTextStream and not with the QTcpSocket...

        PS: Here is my code now for the deletion:

        void MonitorServer::disconnected(){
            _log("disconnected...");
            emit destroyMonitorConnection();
        }
        
        1 Reply Last reply
        0
        • K Offline
          K Offline
          kshegunov
          Moderators
          wrote on 5 Jan 2016, 12:22 last edited by kshegunov 1 May 2016, 12:24
          #11

          Hello,

          It must be silly but I don't see why I'm getting this issue with the QTextStream and not with the QTcpSocket...

          You're getting an error simply because QTextStream is not a QObject subclass, so it doesn't have signals, nor slots and you can't use it with QPointer. QPointer is specifically tailored to be used with QObject pointers, and not with any class. If you wish you could use QScopedPointer for the text stream or just create an instance on the stack when you're reading the socket (which would be my preference). For example:

          void MonitorHandler::readyRead()
          {
              // ... Some code
              QTextStream inputStream(iSocket);
              // ... Read from the stream while there's data pending:
              while (!inputStream.atEnd())  {
                  QString line = inputStream.readLine();
                  // ... Do something with line you've read
              }
          }
          

          Kind regards.

          Read and abide by the Qt Code of Conduct

          M 1 Reply Last reply 5 Jan 2016, 16:49
          0
          • K kshegunov
            5 Jan 2016, 12:22

            Hello,

            It must be silly but I don't see why I'm getting this issue with the QTextStream and not with the QTcpSocket...

            You're getting an error simply because QTextStream is not a QObject subclass, so it doesn't have signals, nor slots and you can't use it with QPointer. QPointer is specifically tailored to be used with QObject pointers, and not with any class. If you wish you could use QScopedPointer for the text stream or just create an instance on the stack when you're reading the socket (which would be my preference). For example:

            void MonitorHandler::readyRead()
            {
                // ... Some code
                QTextStream inputStream(iSocket);
                // ... Read from the stream while there's data pending:
                while (!inputStream.atEnd())  {
                    QString line = inputStream.readLine();
                    // ... Do something with line you've read
                }
            }
            

            Kind regards.

            M Offline
            M Offline
            mbruel
            wrote on 5 Jan 2016, 16:49 last edited by
            #12

            @kshegunov
            ok I see, I thought all QT objects where inherited from QObject which in fact doesn't make sense as we can only inherit once from QObject...
            For the QTextStream, using a local object on the stack means it will be initialised every time the socket receive information... I find it a not efficient so I'll keep a normal pointer.
            Thanks for all your advices.
            I start to really love QT :)

            K 1 Reply Last reply 5 Jan 2016, 17:15
            0
            • M mbruel
              5 Jan 2016, 16:49

              @kshegunov
              ok I see, I thought all QT objects where inherited from QObject which in fact doesn't make sense as we can only inherit once from QObject...
              For the QTextStream, using a local object on the stack means it will be initialised every time the socket receive information... I find it a not efficient so I'll keep a normal pointer.
              Thanks for all your advices.
              I start to really love QT :)

              K Offline
              K Offline
              kshegunov
              Moderators
              wrote on 5 Jan 2016, 17:15 last edited by kshegunov 1 May 2016, 17:18
              #13

              @mbruel said:

              as we can only inherit once from QObject...

              This is news to me, how come you can only inherit once? You can subclass QObject as many times as you wish.

              For the QTextStream, using a local object on the stack means it will be initialised every time the socket receive information... I find it a not efficient so I'll keep a normal pointer.

              This point is moot in your case. If you've profiled your code and indeed you're certain that creating a QTextStream on each read is in fact a bottleneck, then and only then you can think about optimization. If you don't wish to create the object everytime just put it globally in your class and on each connect set the QIODevice for the text stream. What's the point in creating the object in the heap really? You gain nothing but an obligation to clean the memory up at some point. Instead you could put a simple QTextStream stream; declaration in your class and call stream.setDevice(socket) when your socket is initialized, like this:

              class MonitorServer : public QObject
              {
                  // ... Put your code 
              private:
                  QTextStream stream;
              };
              
              void MonitorServer::newConnection()
              {
                  QTcpSocket * sock = iServer->nextPendingConnection();
                  if (!sock)  {  // No pending connection (handle error accordingly)
                  }
              
                  // Ready to accept the connection ... set the stream's IO device
                  stream.setDevice(sock);
              }
              
              void MonitorHandler::readyRead()
              {
                  // ... Read ... read ... read
                  if (line->startsWith(sCmds.QUIT, Qt::CaseInsensitive) || line->startsWith(sCmds.EXIT, Qt::CaseInsensitive))  {
                      stream.setDevice(NULL);    //< This will flush the buffer to the device if any data is pending
                      iSocket->disconnectFromHost();
                  }
                  // ... More code
              }
              

              The same goes for the QStrings you're using. QString is implicitly shared so instead of making your code more optimized by creating the QString in the heap, you're gaining absolutely nothing, only potential memory leaks!

              Read and abide by the Qt Code of Conduct

              M 1 Reply Last reply 5 Jan 2016, 17:57
              0
              • K kshegunov
                5 Jan 2016, 17:15

                @mbruel said:

                as we can only inherit once from QObject...

                This is news to me, how come you can only inherit once? You can subclass QObject as many times as you wish.

                For the QTextStream, using a local object on the stack means it will be initialised every time the socket receive information... I find it a not efficient so I'll keep a normal pointer.

                This point is moot in your case. If you've profiled your code and indeed you're certain that creating a QTextStream on each read is in fact a bottleneck, then and only then you can think about optimization. If you don't wish to create the object everytime just put it globally in your class and on each connect set the QIODevice for the text stream. What's the point in creating the object in the heap really? You gain nothing but an obligation to clean the memory up at some point. Instead you could put a simple QTextStream stream; declaration in your class and call stream.setDevice(socket) when your socket is initialized, like this:

                class MonitorServer : public QObject
                {
                    // ... Put your code 
                private:
                    QTextStream stream;
                };
                
                void MonitorServer::newConnection()
                {
                    QTcpSocket * sock = iServer->nextPendingConnection();
                    if (!sock)  {  // No pending connection (handle error accordingly)
                    }
                
                    // Ready to accept the connection ... set the stream's IO device
                    stream.setDevice(sock);
                }
                
                void MonitorHandler::readyRead()
                {
                    // ... Read ... read ... read
                    if (line->startsWith(sCmds.QUIT, Qt::CaseInsensitive) || line->startsWith(sCmds.EXIT, Qt::CaseInsensitive))  {
                        stream.setDevice(NULL);    //< This will flush the buffer to the device if any data is pending
                        iSocket->disconnectFromHost();
                    }
                    // ... More code
                }
                

                The same goes for the QStrings you're using. QString is implicitly shared so instead of making your code more optimized by creating the QString in the heap, you're gaining absolutely nothing, only potential memory leaks!

                M Offline
                M Offline
                mbruel
                wrote on 5 Jan 2016, 17:57 last edited by
                #14

                @kshegunov said:

                You can subclass QObject as many times as you wish.

                Well that was my impression. For what I remember when I started my project I couldn't inherit from both QTcpSocket and QThread... I don't remember the reason, I thought it had to do with the fact I was inheriting twice from QObject... Maybe it was not that...

                @kshegunov said:

                What's the point in creating the object in the heap really? You gain nothing but an obligation to clean the memory up at some point. Instead you could put a simple QTextStream stream; declaration in your class and call stream.setDevice(socket) when your socket is initialized, like this:

                Well that was my first intention but I looked at the doc of setDevice which states that when the QTextStream is reassigned there is a flush on the old device. I guess there is a test to check if the device exists and is opened but I was lazy to check so I went for a new one in the heap for each connection as the old socket would have been destroyed.
                Now that my project is nearly finished and quite tested, I may go back to this as indeed it is more clean.

                @kshegunov said:

                so instead of making your code more optimized by creating the QString in the heap, you're gaining absolutely nothing, only potential memory leaks!

                I'm not using any QString on the heap.... The example I've put in my 3rd of 4th was just to attempts to identify the cause of the crash when my socket was closing... It was just some debug tests. By using your solution to queue the disconnect call, I went back to a normal local string on the stack.

                I'll have a read of your links.
                Cheers

                K 1 Reply Last reply 5 Jan 2016, 18:08
                0
                • M mbruel
                  5 Jan 2016, 17:57

                  @kshegunov said:

                  You can subclass QObject as many times as you wish.

                  Well that was my impression. For what I remember when I started my project I couldn't inherit from both QTcpSocket and QThread... I don't remember the reason, I thought it had to do with the fact I was inheriting twice from QObject... Maybe it was not that...

                  @kshegunov said:

                  What's the point in creating the object in the heap really? You gain nothing but an obligation to clean the memory up at some point. Instead you could put a simple QTextStream stream; declaration in your class and call stream.setDevice(socket) when your socket is initialized, like this:

                  Well that was my first intention but I looked at the doc of setDevice which states that when the QTextStream is reassigned there is a flush on the old device. I guess there is a test to check if the device exists and is opened but I was lazy to check so I went for a new one in the heap for each connection as the old socket would have been destroyed.
                  Now that my project is nearly finished and quite tested, I may go back to this as indeed it is more clean.

                  @kshegunov said:

                  so instead of making your code more optimized by creating the QString in the heap, you're gaining absolutely nothing, only potential memory leaks!

                  I'm not using any QString on the heap.... The example I've put in my 3rd of 4th was just to attempts to identify the cause of the crash when my socket was closing... It was just some debug tests. By using your solution to queue the disconnect call, I went back to a normal local string on the stack.

                  I'll have a read of your links.
                  Cheers

                  K Offline
                  K Offline
                  kshegunov
                  Moderators
                  wrote on 5 Jan 2016, 18:08 last edited by
                  #15

                  @mbruel
                  Hello,

                  Well that was my impression. For what I remember when I started my project I couldn't inherit from both QTcpSocket and QThread... I don't remember the reason, I thought it had to do with the fact I was inheriting twice from QObject... Maybe it was not that...

                  I guess there was a bit of misunderstanding here. I meant that you can construct a hierarchy based on QObject without any restriction on the depth. You were talking of multiple inheritance. With multiple inheritance you should not extend two classes that are from the same hierarchy (with some very fine exceptions to that rule) and if in the end you do that should be a virtual inheritance like this:

                  class BadClass : public virtual BaseClass1, public virtual BaseClass2  {};
                  

                  In any case you shouldn't be subclassing QThread not to mention both QThread and QTcpSocket. Your new class is not a thread and a socket at the same time, is it?

                  I'm not using any QString on the heap.... The example I've put in my 3rd of 4th was just to attempts to identify the cause of the crash when my socket was closing... It was just some debug tests.

                  Okay, in that case you can disregard the comment. Still, in my opinion, one should be striving to use as little heap allocations as possible. In the end each QObject is dragging a private object that is allocated in the heap, and each implicitly shared class is doing the same but with a shared data object. So if you have a class that inherits from QObject there is nothing wrong in declaring your variables directly, instead of creating them in the heap and using the returned pointers.

                  Kind regards.

                  Read and abide by the Qt Code of Conduct

                  M 1 Reply Last reply 5 Jan 2016, 20:34
                  0
                  • K kshegunov
                    5 Jan 2016, 18:08

                    @mbruel
                    Hello,

                    Well that was my impression. For what I remember when I started my project I couldn't inherit from both QTcpSocket and QThread... I don't remember the reason, I thought it had to do with the fact I was inheriting twice from QObject... Maybe it was not that...

                    I guess there was a bit of misunderstanding here. I meant that you can construct a hierarchy based on QObject without any restriction on the depth. You were talking of multiple inheritance. With multiple inheritance you should not extend two classes that are from the same hierarchy (with some very fine exceptions to that rule) and if in the end you do that should be a virtual inheritance like this:

                    class BadClass : public virtual BaseClass1, public virtual BaseClass2  {};
                    

                    In any case you shouldn't be subclassing QThread not to mention both QThread and QTcpSocket. Your new class is not a thread and a socket at the same time, is it?

                    I'm not using any QString on the heap.... The example I've put in my 3rd of 4th was just to attempts to identify the cause of the crash when my socket was closing... It was just some debug tests.

                    Okay, in that case you can disregard the comment. Still, in my opinion, one should be striving to use as little heap allocations as possible. In the end each QObject is dragging a private object that is allocated in the heap, and each implicitly shared class is doing the same but with a shared data object. So if you have a class that inherits from QObject there is nothing wrong in declaring your variables directly, instead of creating them in the heap and using the returned pointers.

                    Kind regards.

                    M Offline
                    M Offline
                    mbruel
                    wrote on 5 Jan 2016, 20:34 last edited by mbruel 1 May 2016, 20:36
                    #16

                    @kshegunov

                    In any case you shouldn't be subclassing QThread not to mention both QThread and QTcpSocket. Your new class is not a thread and a socket at the same time, is it?

                    Well it was before knowing how QThread works, I thought the whole object, including its slots would be in the thread. That's why I wanted to have my Connection class to be a threaded socket. After a bit of reading I realised it doesn't work like this but that we should moveToThread instead.

                    @kshegunov

                    So if you have a class that inherits from QObject there is nothing wrong in declaring your variables directly, instead of creating them in the heap and using the returned pointers.

                    Yeah that's true. But pointers are still needed when you want to extend the scope of the objects and share them between different objects or also just have an handle independently of the instance (QTcpSocket handle pointing on a QTcpSocket or a QSslSocket)

                    I've just read your link about Implicit sharing, that's interesting to know. There are not so many classes but yeah QByteArray and QString are quite common. I suppose my crash was due to this sharing even between a QString built on a implicit shared QByteArray. I would have expect thus that there would be a copy done at detach time when the source QByteArray is destroyed but still have a local QString using the data...

                    K 1 Reply Last reply 5 Jan 2016, 20:44
                    0
                    • M mbruel
                      5 Jan 2016, 20:34

                      @kshegunov

                      In any case you shouldn't be subclassing QThread not to mention both QThread and QTcpSocket. Your new class is not a thread and a socket at the same time, is it?

                      Well it was before knowing how QThread works, I thought the whole object, including its slots would be in the thread. That's why I wanted to have my Connection class to be a threaded socket. After a bit of reading I realised it doesn't work like this but that we should moveToThread instead.

                      @kshegunov

                      So if you have a class that inherits from QObject there is nothing wrong in declaring your variables directly, instead of creating them in the heap and using the returned pointers.

                      Yeah that's true. But pointers are still needed when you want to extend the scope of the objects and share them between different objects or also just have an handle independently of the instance (QTcpSocket handle pointing on a QTcpSocket or a QSslSocket)

                      I've just read your link about Implicit sharing, that's interesting to know. There are not so many classes but yeah QByteArray and QString are quite common. I suppose my crash was due to this sharing even between a QString built on a implicit shared QByteArray. I would have expect thus that there would be a copy done at detach time when the source QByteArray is destroyed but still have a local QString using the data...

                      K Offline
                      K Offline
                      kshegunov
                      Moderators
                      wrote on 5 Jan 2016, 20:44 last edited by
                      #17

                      @mbruel

                      Yeah that's true. But pointers are still needed when you want to extend the scope of the objects and share them between different objects or also just have an handle independently of the instance (QTcpSocket handle pointing on a QTcpSocket or a QSslSocket)

                      I don't argue that. But in your case the QTextStream is locally used, so to allocate in the heap is not as beneficial. In any case this is mostly depending on your preference (i.e. moc generates everything in the heap), I was just giving my opinion.

                      There are not so many classes but yeah QByteArray and QString are quite common.

                      In fact many classes in Qt are implicitly shared, including all the container (QList, QVector and the like) the images/pixmaps (QImage, QPixmap), strings and byte arrays, if memory serves me QVariant and more.

                      I suppose my crash was due to this sharing even between a QString built on a implicit shared QByteArray.

                      I doubt it. Probably your stack got corrupted and that's why you got the crash. Implicitly shared classes (as to my knowledge) are detaching-thread-safe - meaning that the data copying (which is occurring internally in the background) is thread safe.

                      Read and abide by the Qt Code of Conduct

                      1 Reply Last reply
                      0

                      17/17

                      5 Jan 2016, 20:44

                      • Login

                      • Login or register to search.
                      17 out of 17
                      • First post
                        17/17
                        Last post
                      0
                      • Categories
                      • Recent
                      • Tags
                      • Popular
                      • Users
                      • Groups
                      • Search
                      • Get Qt Extensions
                      • Unsolved