Cleanly closing a QSerialPort in Qt

Question

I am trying to close a serial port opened using the QSerialPort library but it hangs more than half the time.

I am developing a multi-threaded app, with one thread responsible for UI and the other for serial communication. I am using the QThread wrapper class.

    void CommThread::run()
{
    serial = new QSerialPort();

    serial->setPortName(portname);
    serial->setBaudRate(QSerialPort::Baud115200);

    if(!serial->open(QIODevice::ReadWrite)){
        qDebug() << "Error opening Serial port within thread";
        quit = true;
        return;
    }else{
        /// \todo handle this exception more gracefully
    }

    /// Start our reading loop
    /// While CommThread::disconnect is not called, this loop will run
    while(!quit){
        comm_mutex->lock();

        /// If CommThread::disconnect() is called send DISCONNECT Packet
        if(dconnect){
            // Signal device to disconnect so that it can suspend USB CDC transmission of data
            qDebug() << "Entering disconnect sequence";

            serial->write(data);
            serial->flush();

            break;
        }

        /// No write or disconnect requested
        /// Read incoming data from port
        if(serial->waitForReadyRead(-1)){
            if(serial->canReadLine()){
              // Read stuff here
            }
        }

        // Transform the stuff read here

        comm_mutex->lock()
        // Do something to a shared data structure 
        // emit signal to main thread that data is ready           
        comm_mutex->unlock();
    }

    comm_mutex->unlock();

    // Thread is exiting, clean up resources it created
    qDebug() << "Thread ID" << QThread::currentThreadId();
    qDebug() << "Thread:: Closing and then deleting the serial port";
    qDebug() << "Lets check the error string" << serial->errorString();
    delete comm_mutex;
    serial->close();
    qDebug() << "Thread:: Port closed";
    delete serial;
    qDebug() << "Thread:: Serial deleted";
    delete img;
    qDebug() << "Thread:: Image deleted";
    qDebug() << "Thread:: Serial port and img memory deleted";
    quit = true;

}

The problem is when the UI thread sets the dconnect variable to true and proceeds to delete the communication thread it gets stuck in the destructor of the communication thread which looks like this:

    CommThread::~CommThread()
{
    qDebug() << "Destructor waiting for thread to stop";
    QThread::wait();
    qDebug() << "Destuctor Commthread ID" << QThread::currentThreadId();
    qDebug() << "Commthread wrapper exiting";
}

2 out of three times, the communication thread hangs at the serial-close() line, causing the UI thread to hang at the QThread::wait() line in the destructor. Needless to say this results in a frozen UI and if closed, the entire application remains in memory until killed by the task manager. Given a few minutes the call to serial::close() will finally return; what I would like to know is what's wrong and how can I best avoid a hanging UI?

I have looked into the code of QSerialPort and I can't see anything manifestly wrong. If I call serial->errorCode() I get the UknownError string but that happens even when the port closes with no hangups.

EDIT: This NEVER happens in the debugger. The SerialPort always closes immediately and the destructor sails through with no hangups on QThread::wait()

EDIT: I am certain it is the serial->close() which is hanging because I can see the qDebug() statement being printed just before it hangs for several seconds or minutes).

The device stops transmitting because in the dconnect switch, a disconnect packet is sent and a LED on the device turns green.

Answer 1

Several things:

1. You can certainly simply leak the port if it doesn't close soon enough.

2. You should perform a graceful exit where the UI is responsive and the thread shutdown is attempted with a timeout.

3. You should use smart pointers and other RAII techniques to manage resources. This is C++, not C. Ideally, store things by value, not through a pointer.

4. You must not block in the of the sections where you modify the shared data structure(s) under a lock.

5. You should be notifying of changes to the data structure (perhaps you do). How can other code depend on such changes without polling otherwise? It can't, and polling is horrible for performance.

6. QThread offers requestInterruption and isInterruptionRequested for code that reimplements run without an event loop. Use it, don't roll your won quit flags.

7. Your code would be much simpler if you had used a QObject directly.

At the very minimum, we want a UI that won't block on a worker thread being shut down. We start with a thread implementation that has the functionality needed to support such a UI.

// https://github.com/KubaO/stackoverflown/tree/master/questions/serial-test-32331713
#include <QtWidgets>

/// A thread that gives itself a bit of time to finish up, and then terminates.
class Thread : public QThread {
   Q_OBJECT
   Q_PROPERTY (int shutdownTimeout MEMBER m_shutdownTimeout)
   int m_shutdownTimeout { 1000 }; ///< in milliseconds
   QBasicTimer m_shutdownTimer;
   void timerEvent(QTimerEvent * ev) override {
      if (ev->timerId() == m_shutdownTimer.timerId()) {
         if (! isFinished()) terminate();
      }
      QThread::timerEvent(ev);
   }
   bool event(QEvent *event) override {
      if (event->type() == QEvent::ThreadChange)
         QCoreApplication::postEvent(this, new QEvent(QEvent::None));
      else if (event->type() == QEvent::None && thread() == currentThread())
         // Hint that moveToThread(this) is an antipattern
         qWarning() << "The thread controller" << this << "is running in its own thread.";
      return QThread::event(event);
   }
   using QThread::requestInterruption; ///< Hidden, use stop() instead.
   using QThread::quit; ///< Hidden, use stop() instead.
public:
   Thread(QObject * parent = 0) : QThread(parent) {
      connect(this, &QThread::finished, this, [this]{ m_shutdownTimer.stop(); });
   }
   /// Indicates that the thread is attempting to finish.
   Q_SIGNAL void stopping();
   /// Signals the thread to stop in a general way.
   Q_SLOT void stop() {
      emit stopping();
      m_shutdownTimer.start(m_shutdownTimeout, this);
      requestInterruption(); // should break a run() that has no event loop
      quit();                // should break the event loop if there is one
   }
   ~Thread() {
      Q_ASSERT(!thread() || thread() == QThread::currentThread());
      stop();
      wait(50);
      if (isRunning()) terminate();
      wait();
   }
};

It's a bit of a lie that Thread is-a QThread since we cannot use some of the base class's members on it, thus breaking the LSP. Ideally, Thread should be a QObject, and only internally contain a QThread.

We then implement a dummy thread that takes its time to terminate, and can optionally get stuck permanently, just like your code sometime does (although it doesn't have to).

class LazyThread : public Thread {
   Q_OBJECT
   Q_PROPERTY(bool getStuck MEMBER m_getStuck)
   bool m_getStuck { false };
   void run() override {
      while (!isInterruptionRequested()) {
         msleep(100); // pretend that we're busy
      }
      qDebug() << "loop exited";
      if (m_getStuck) {
         qDebug() << "stuck";
         Q_FOREVER sleep(1);
      } else {
         qDebug() << "a little nap";
         sleep(2);
      }
   }
public:
   LazyThread(QObject * parent = 0) : Thread(parent) {
      setProperty("shutdownTimeout", 5000);
   }
};

We then need a class that can link up worker threads and UI close requests. It installs itself as an event filter on the main window, and delays its closing until all threads have terminated.

class CloseThreadStopper : public QObject {
   Q_OBJECT
   QSet<Thread*> m_threads;
   void done(Thread* thread ){
      m_threads.remove(thread);
      if (m_threads.isEmpty()) emit canClose();
   }
   bool eventFilter(QObject * obj, QEvent * ev) override {
      if (ev->type() == QEvent::Close) {
         bool close = true;
         for (auto thread : m_threads) {
            if (thread->isRunning() && !thread->isFinished()) {
               close = false;
               ev->ignore();
               connect(thread, &QThread::finished, this, [this, thread]{ done(thread); });
               thread->stop();
            }
         }
         return !close;
      }
      return false;
   }
public:
   Q_SIGNAL void canClose();
   CloseThreadStopper(QObject * parent = 0) : QObject(parent) {}
   void addThread(Thread* thread) {
      m_threads.insert(thread);
      connect(thread, &QObject::destroyed, this, [this, thread]{ done(thread); });
   }
   void installOn(QWidget * w) {
      w->installEventFilter(this);
      connect(this, &CloseThreadStopper::canClose, w, &QWidget::close);
   }
};

Finally, we have a simple UI that allows us to control all this and see that it works. At no point is the UI unresponsive or blocked.

int main(int argc, char *argv[])
{
   QApplication a { argc, argv };
   LazyThread thread;
   CloseThreadStopper stopper;
   stopper.addThread(&thread);

   QWidget ui;
   QGridLayout layout { &ui };
   QLabel state;
   QPushButton start { "Start" }, stop { "Stop" };
   QCheckBox stayStuck { "Keep the thread stuck" };
   layout.addWidget(&state, 0, 0, 1, 2);
   layout.addWidget(&stayStuck, 1, 0, 1, 2);
   layout.addWidget(&start, 2, 0);
   layout.addWidget(&stop, 2, 1);
   stopper.installOn(&ui);
   QObject::connect(&stayStuck, &QCheckBox::toggled, &thread, [&thread](bool v){
      thread.setProperty("getStuck", v);
   });

   QStateMachine sm;
   QState s_started { &sm }, s_stopping { &sm }, s_stopped { &sm };
   sm.setGlobalRestorePolicy(QState::RestoreProperties);
   s_started.assignProperty(&state, "text", "Running");
   s_started.assignProperty(&start, "enabled", false);
   s_stopping.assignProperty(&state, "text", "Stopping");
   s_stopping.assignProperty(&start, "enabled", false);
   s_stopping.assignProperty(&stop, "enabled", false);
   s_stopped.assignProperty(&state, "text", "Stopped");
   s_stopped.assignProperty(&stop, "enabled", false);

   for (auto state : { &s_started, &s_stopping })
      state->addTransition(&thread, SIGNAL(finished()), &s_stopped);
   s_started.addTransition(&thread, SIGNAL(stopping()), &s_stopping);
   s_stopped.addTransition(&thread, SIGNAL(started()), &s_started);
   QObject::connect(&start, &QPushButton::clicked, [&]{ thread.start(); });
   QObject::connect(&stop, &QPushButton::clicked, &thread, &Thread::stop);
   sm.setInitialState(&s_stopped);

   sm.start();
   ui.show();
   return a.exec();
}

#include "main.moc"

Given the Thread class, and following advice above (other than point 7), your run() should look roughly as follows:

class CommThread : public Thread {
   Q_OBJECT
public:
   enum class Request { Disconnect };
private:
   QMutex m_mutex;
   QQueue<Request> m_requests;
   //...
   void run() override;
};

void CommThread::run()
{
   QString portname;
   QSerialPort port;

   port.setPortName(portname);
   port.setBaudRate(QSerialPort::Baud115200);

   if (!port.open(QIODevice::ReadWrite)){
      qWarning() << "Error opening Serial port within thread";
      return;
   }

   while (! isInterruptionRequested()) {
      QMutexLocker lock(&m_mutex);
      if (! m_requests.isEmpty()) {
         auto request = m_requests.dequeue();
         lock.unlock();
         if (request == Request::Disconnect) {
            qDebug() << "Entering disconnect sequence";
            QByteArray data;
            port.write(data);
            port.flush();
         }
         //...
      }
      lock.unlock();

      // The loop must run every 100ms to check for new requests
      if (port.waitForReadyRead(100)) {
         if (port.canReadLine()) {
            //...
         }
         QMutexLocker lock(&m_mutex);
         // Do something to a shared data structure
      }

      qDebug() << "The thread is exiting";
   }
}

Of course, this is a truly horrible style that unnecessarily spins the loop waiting for things to happen, etc. Instead, the trivial way to approach such issues is to have a QObject with a thread-safe interface that can be moved to a worker thread.

First, a curiously recurring helper; see this question for details.

namespace {
template <typename F>
static void postTo(QObject * obj, F && fun) {
   QObject signalSource;
   QObject::connect(&signalSource, &QObject::destroyed, obj, std::forward<F>(fun),
                    Qt::QueuedConnection);
}
}

We derive from QObject and use postTo to execute functors from our thread's event loop.

class CommObject : public QObject {
   Q_OBJECT
   Q_PROPERTY(QImage image READ image NOTIFY imageChanged)
   mutable QMutex m_imageMutex;
   QImage m_image;
   QByteArray m_data;
   QString m_portName;
   QSerialPort m_port { this };
   void onData() {
      if (m_port.canReadLine()) {
         // process the line
      }
      QMutexLocker lock(&m_imageMutex);
      // Do something to the image
      emit imageChanged(m_image);
   }
public:
   /// Thread-safe
   Q_SLOT void disconnect() {
      postTo(this, [this]{
         qDebug() << "Entering disconnect sequence";
         m_port.write(m_data);
         m_port.flush();
      });
   }
   /// Thread-safe
   Q_SLOT void open() {
      postTo(this, [this]{
         m_port.setPortName(m_portName);
         m_port.setBaudRate(QSerialPort::Baud115200);
         if (!m_port.open(QIODevice::ReadWrite)){
            qWarning() << "Error opening the port";
            emit openFailed();
         } else {
            emit opened();
         }
      });
   }
   Q_SIGNAL void opened();
   Q_SIGNAL void openFailed();
   Q_SIGNAL void imageChanged(const QImage &);
   CommObject(QObject * parent = 0) : QObject(parent) {
      open();
      connect(&m_port, &QIODevice::readyRead, this, &CommObject::onData);
   }
   QImage image() const {
      QMutexLocker lock(&m_imageMutex);
      return m_image;
   }
};

Let's observe that any QIODevice automatically closes on destruction. Thus all we need to do to close the port is to destruct it, in the desired worker thread so that the long operation doesn't block the UI.

Thus, we really want the object (and its port) to be deleted in its thread (or leak). This is simply accomplished by connecting Thread::stopping to the object's deleteLater slot. There, the port closing can take as much time as needed - the Thread will terminate its execution if it times out. All the while the UI remains responsive.

int main(...) {
  //...
  Thread thread;
  thread.start();
  QScopedPointer<CommObject> comm(new CommObject);
  comm->moveToThread(&thread);
  QObject::connect(&thread, &Thread::stopping, comm.take(), &QObject::deleteLater);
  //...
}