From e0399d31a69433b96a36f9ed0de94802219d8327 Mon Sep 17 00:00:00 2001 From: Norbert Truchsess Date: Fri, 11 Aug 2023 21:50:48 +0200 Subject: [PATCH 1/2] QMS-624 fix BRouter mutex threading issues --- .../gis/rte/router/CRouterBRouter.cpp | 46 +++++++++++++------ src/qmapshack/gis/rte/router/CRouterBRouter.h | 7 ++- .../router/brouter/CRouterBRouterLocal.cpp | 3 +- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/qmapshack/gis/rte/router/CRouterBRouter.cpp b/src/qmapshack/gis/rte/router/CRouterBRouter.cpp index 9a56053fc..4805fec2b 100644 --- a/src/qmapshack/gis/rte/router/CRouterBRouter.cpp +++ b/src/qmapshack/gis/rte/router/CRouterBRouter.cpp @@ -64,6 +64,10 @@ CRouterBRouter::CRouterBRouter(QWidget* parent) : IRouter(false, parent) { timerCloseStatusMsg->setInterval(5000); connect(timerCloseStatusMsg, &QTimer::timeout, this, &CRouterBRouter::slotCloseStatusMsg); + timerSynchronousRequest = new QTimer(this); + timerSynchronousRequest->setSingleShot(true); + timerSynchronousRequest->setInterval(300); + routerSetup = dynamic_cast(parent); connect(toolConsole, &QToolButton::clicked, this, &CRouterBRouter::slotToggleConsole); @@ -298,32 +302,38 @@ int CRouterBRouter::calcRoute(const QPointF& p1, const QPointF& p2, QPolygonF& c int CRouterBRouter::synchronousRequest(const QVector& points, const QList& nogos, QPolygonF& coords, qreal* costs = nullptr) noexcept(false) { - if (!mutex.tryLock()) { - // skip further on-the-fly-requests as long a previous request is still running + if (isCalculating) { + if (!timerSynchronousRequest->isActive()) { + emit sigCancelRouting(); + } return -1; } - + timerSynchronousRequest->start(); + isCalculating = true; + if (setup->installMode == CRouterBRouterSetup::eModeLocal && localBRouter->isBRouterNotRunning()) { localBRouter->startBRouter(); } - synchronous = true; - QNetworkReply* reply = networkAccessManager->get(getRequest(points, nogos)); try { + reply->setProperty("synchronous", true); reply->setProperty("options", getOptions()); reply->setProperty("time", QDateTime::currentDateTimeUtc().toMSecsSinceEpoch()); CProgressDialog progress(tr("Calculate route with %1").arg(getOptions()), 0, NOINT, nullptr); QEventLoop eventLoop; + connect(this, &CRouterBRouter::sigCancelRouting, reply, &QNetworkReply::abort); connect(&progress, &CProgressDialog::rejected, reply, &QNetworkReply::abort); connect(reply, &QNetworkReply::finished, &eventLoop, &QEventLoop::quit); eventLoop.exec(QEventLoop::AllEvents); const QNetworkReply::NetworkError& netErr = reply->error(); - if (netErr == QNetworkReply::RemoteHostClosedError && nogos.size() > 1 && !isMinimumVersion(1, 4, 10)) { + if (netErr == QNetworkReply::OperationCanceledError) { + throw QString(); + } else if (netErr == QNetworkReply::RemoteHostClosedError && nogos.size() > 1 && !isMinimumVersion(1, 4, 10)) { throw tr("this version of BRouter does not support more then 1 nogo-area"); } else if (netErr != QNetworkReply::NoError) { throw reply->errorString(); @@ -381,8 +391,10 @@ int CRouterBRouter::synchronousRequest(const QVector& points, const QLi } } } catch (const QString& msg) { + coords.clear(); reply->deleteLater(); - mutex.unlock(); + timerSynchronousRequest->stop(); + isCalculating = false; if (!msg.isEmpty()) { throw tr("Bad response from server: %1").arg(msg); } @@ -391,18 +403,23 @@ int CRouterBRouter::synchronousRequest(const QVector& points, const QLi reply->deleteLater(); slotCloseStatusMsg(); - mutex.unlock(); + timerSynchronousRequest->stop(); + isCalculating = false; return coords.size(); } void CRouterBRouter::calcRoute(const IGisItem::key_t& key) { - mutex.lock(); + if (isCalculating) { + return; + } + isCalculating = true; + if (setup->installMode == CRouterBRouterSetup::eModeLocal && localBRouter->isBRouterNotRunning()) { localBRouter->startBRouter(); } + CGisItemRte* rte = dynamic_cast(CGisWorkspace::self().getItemByKey(key)); if (nullptr == rte) { - mutex.unlock(); return; } @@ -418,10 +435,9 @@ void CRouterBRouter::calcRoute(const IGisItem::key_t& key) { points << QPointF(pt.lon, pt.lat); } - synchronous = false; - QNetworkReply* reply = networkAccessManager->get(getRequest(points, nogos)); + reply->setProperty("synchronous", false); reply->setProperty("key.item", key.item); reply->setProperty("key.project", key.project); reply->setProperty("key.device", key.device); @@ -440,7 +456,7 @@ void CRouterBRouter::calcRoute(const IGisItem::key_t& key) { } void CRouterBRouter::slotRequestFinished(QNetworkReply* reply) { - if (synchronous) { + if (reply->property("synchronous").toBool()) { return; } @@ -492,13 +508,13 @@ void CRouterBRouter::slotRequestFinished(QNetworkReply* reply) { } timerCloseStatusMsg->start(); reply->deleteLater(); - mutex.unlock(); + isCalculating = false; return; } } slotCloseStatusMsg(); - mutex.unlock(); + isCalculating = false; } void CRouterBRouter::slotToggleConsole() const { diff --git a/src/qmapshack/gis/rte/router/CRouterBRouter.h b/src/qmapshack/gis/rte/router/CRouterBRouter.h index 4ccfb50b7..518560305 100644 --- a/src/qmapshack/gis/rte/router/CRouterBRouter.h +++ b/src/qmapshack/gis/rte/router/CRouterBRouter.h @@ -51,6 +51,9 @@ class CRouterBRouter : public IRouter, private Ui::IRouterBRouter { void setupLocalDir(QString localDir); + signals: + void sigCancelRouting(); + public slots: void slotToolSetupClicked(); @@ -79,8 +82,8 @@ class CRouterBRouter : public IRouter, private Ui::IRouterBRouter { QNetworkAccessManager* networkAccessManager; QTimer* timerCloseStatusMsg; - bool synchronous = false; - QMutex mutex; + bool isCalculating{false}; + QTimer* timerSynchronousRequest; CRouterBRouterSetup* setup; CRouterSetup* routerSetup; CRouterBRouterInfo* info; diff --git a/src/qmapshack/gis/rte/router/brouter/CRouterBRouterLocal.cpp b/src/qmapshack/gis/rte/router/brouter/CRouterBRouterLocal.cpp index 13780cd32..e7b9013d6 100644 --- a/src/qmapshack/gis/rte/router/brouter/CRouterBRouterLocal.cpp +++ b/src/qmapshack/gis/rte/router/brouter/CRouterBRouterLocal.cpp @@ -90,8 +90,7 @@ void CRouterBRouterLocal::startBRouter() { while (timer.remainingTime() > 0 && brouterState == QProcess::Running) { socket.connectToHost(brouter.setup->localHost, brouter.setup->localPort.toInt()); - // Processing userinputevents in local eventloop would cause a SEGV when clicking 'abort' of calling LineOp - connectState = connect_state_e(eventLoop->exec(QEventLoop::ExcludeUserInputEvents)); + connectState = connect_state_e(eventLoop->exec(QEventLoop::AllEvents)); // retry after 100ms, but only in case socket is not yet connectable if (connectState == eError && socketError == QAbstractSocket::ConnectionRefusedError) { From a35aba285d7cf5f4570eea0b9a020bb2b1958a2f Mon Sep 17 00:00:00 2001 From: Norbert Truchsess Date: Tue, 15 Aug 2023 17:27:41 +0200 Subject: [PATCH 2/2] QMS-624 skip full canvas update in BRouters routing-result processing --- src/qmapshack/gis/rte/router/CRouterBRouter.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qmapshack/gis/rte/router/CRouterBRouter.cpp b/src/qmapshack/gis/rte/router/CRouterBRouter.cpp index 4805fec2b..69741bd13 100644 --- a/src/qmapshack/gis/rte/router/CRouterBRouter.cpp +++ b/src/qmapshack/gis/rte/router/CRouterBRouter.cpp @@ -185,7 +185,6 @@ void CRouterBRouter::slotCloseStatusMsg() const { timerCloseStatusMsg->stop(); CCanvas* canvas = CMainWindow::self().getVisibleCanvas(); if (canvas) { - canvas->slotTriggerCompleteUpdate(CCanvas::eRedrawGis); canvas->reportStatus("BRouter", ""); } } @@ -446,7 +445,6 @@ void CRouterBRouter::calcRoute(const IGisItem::key_t& key) { CCanvas* canvas = CMainWindow::self().getVisibleCanvas(); if (canvas) { - canvas->slotTriggerCompleteUpdate(CCanvas::eRedrawGis); canvas->reportStatus("BRouter", tr("BRouter
Routing request sent to server. Please wait...")); }