danvratil / qcoro

C++ Coroutines for Qt
https://qcoro.dvratil.cz
MIT License
326 stars 53 forks source link

Bug: application crashes when signal from another thread timeouts #213

Closed Kaltxi closed 5 months ago

Kaltxi commented 6 months ago

Description:

In the case when we use co_await qCoro(obj, &Obj::signal, timeout) when obj was moved to another thread and the await results in a timeout, the application crashes with a bunch of warnings, such as:

QObject::setParent: Cannot set parent, new parent is in a different thread
QBasicTimer::stop: Failed. Possibly trying to stop from a different thread
QBasicTimer::start: Timers cannot be started from another thread

Example for reproduction

Here is a minimalish Qt5 example (I tried to make a simple console example, but it doesn't behave as I expect, so I had to complicate it a bit with GUI, but now it resembles my original problem pretty well):

#include <QApplication>
#include <QCoro/Task>
#include <QCoroCore>
#include <QDebug>
#include <QMainWindow>
#include <QMessageBox>
#include <QThread>

using namespace std::chrono_literals;

struct Worker final : QObject {
  Q_OBJECT;

public:
  explicit Worker(QObject* parent = nullptr) : QObject(parent) {}

signals:
  void never_emmited();
};

auto listen_to(Worker* worker) -> QCoro::Task<> {
  const auto result = co_await qCoro(worker, &Worker::never_emmited, 1s);
  if (result.has_value()) {
    qDebug() << "Received value!\n";
  } else {
    QMessageBox::warning(nullptr, "Timeout", "Timeout!");
    qDebug() << "Timeout!\n";
  }

  co_return;
}

auto main(int argc, char* argv[]) -> int {
  QApplication app(argc, argv);
  auto* window = new QMainWindow;
  window->show();

  auto* producer_thread = new QThread;
  auto* producer = new Worker;
  producer->moveToThread(producer_thread);
  producer_thread->start();

  listen_to(producer);

  QTimer::singleShot(5s, [=]() {
    producer_thread->quit();
    producer_thread->wait();
    QApplication::quit();
  });

  return QApplication::exec();
}

#include "main.moc"

After running it the following will happen:

  1. The window will pop up.
  2. After one second warning dialog will pop up and crash the application.
  3. In the log there will be no Timeout!, but instead some warnings from Qt.

The QMessageBox is vital here, and that's the part I am not entirely sure why, perhaps because it spins a new event loop and pauses the previous one? If we remove it, or remove the moveToThread - everything works as expected.

Potential cause

Reviewing the code, I see that the following might be the cause: https://github.com/danvratil/qcoro/blob/0006e41682a24b78a1dafe41a6e233a23ce16cea/qcoro/core/qcorosignal.h#L111-L120

Here the mObj is the receiver, while mTimeoutTimer is the emitter and Qt::AutoConnection is used by default. In the situation that mObj is in different thread, the slot is executed via Qt::QueuedConnection - on the receiver's thread and the coroutine will be resumed from different thread than the one coroutine was lauched in. That said I'm not very skilled in coroutines at the moment, so I might be wrong.

danvratil commented 6 months ago

Indeed, cross-thread support for qCoro() is not well tested, thanks for the report. I'll look into it asap.