From 517afdfff3dafddc7ebe6b050c2c8ae7a1b4fb8b Mon Sep 17 00:00:00 2001 From: isanae <14251494+isanae@users.noreply.github.com> Date: Fri, 12 Jul 2019 01:47:59 -0400 Subject: [PATCH 1/3] make sure timers don't fire after restart MO fixes crash when request times out after the restart --- src/github.cpp | 44 +++++++++++++++++++++++++++++++++++++++----- src/github.h | 6 ++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/github.cpp b/src/github.cpp index 4215c66..4c67aec 100644 --- a/src/github.cpp +++ b/src/github.cpp @@ -21,6 +21,16 @@ GitHub::GitHub(const char *clientId) } } +GitHub::~GitHub() +{ + // delete all the replies since they depend on the access manager, which is + // about to be deleted + for (auto* reply : m_replies) { + reply->disconnect(); + delete reply; + } +} + QJsonArray GitHub::releases(const Repository &repo) { QJsonDocument result @@ -114,11 +124,18 @@ void GitHub::request(Method method, const QString &path, const QByteArray &data, const std::function &callback, bool relative) { - QTimer *timer = new QTimer(); + // make sure the timer is a child of this so it's deleted correctly and + // doesn't fire after the GitHub object is destroyed; this happens when + // restarting MO by switching instances, for example + QTimer *timer = new QTimer(this); + timer->setSingleShot(true); timer->setInterval(30000); QNetworkReply *reply = genReply(method, path, data, relative); + // remember this reply + m_replies.push_back(reply); + connect(reply, &QNetworkReply::finished, [this, reply, timer, method, data, callback]() { QJsonDocument result = handleReply(reply); QJsonObject object = result.object(); @@ -129,24 +146,41 @@ void GitHub::request(Method method, const QString &path, const QByteArray &data, } else { callback(result); } - reply->deleteLater(); + + deleteReply(reply); }); connect(reply, static_cast( &QNetworkReply::error), - [reply, timer, callback](QNetworkReply::NetworkError error) { + [this, reply, timer, callback](QNetworkReply::NetworkError error) { qDebug("network error %d", error); timer->stop(); reply->disconnect(); callback(QJsonDocument( QJsonObject({{"network_error", reply->errorString()}}))); - reply->deleteLater(); + + deleteReply(reply); }); - connect(timer, &QTimer::timeout, [reply]() { + connect(timer, &QTimer::timeout, [this, reply]() { qDebug("timeout"); + + // don't delete the reply, abort will fire the error() handler above reply->abort(); }); + timer->start(); } + +void GitHub::deleteReply(QNetworkReply* reply) +{ + // remove from the list + auto itor = std::find(m_replies.begin(), m_replies.end(), reply); + if (itor != m_replies.end()) { + m_replies.erase(itor); + } + + // delete + reply->deleteLater(); +} diff --git a/src/github.h b/src/github.h index 3f26724..9faaca2 100644 --- a/src/github.h +++ b/src/github.h @@ -67,6 +67,7 @@ public: public: GitHub(const char *clientId = nullptr); + ~GitHub(); QJsonArray releases(const Repository &repo); void releases(const Repository &repo, @@ -85,4 +86,9 @@ private: private: QNetworkAccessManager *m_AccessManager; + + // remember the replies that are in flight and delete them in the destructor + std::vector m_replies; + + void deleteReply(QNetworkReply* reply); }; From bb48f0662bf72eac95180bd6cd9ab9baa39bad05 Mon Sep 17 00:00:00 2001 From: isanae <14251494+isanae@users.noreply.github.com> Date: Fri, 12 Jul 2019 02:03:20 -0400 Subject: [PATCH 2/3] split the lambdas into member functions instead --- src/github.cpp | 90 ++++++++++++++++++++++++++++++-------------------- src/github.h | 15 +++++++++ 2 files changed, 69 insertions(+), 36 deletions(-) diff --git a/src/github.cpp b/src/github.cpp index 4c67aec..9bdc766 100644 --- a/src/github.cpp +++ b/src/github.cpp @@ -1,8 +1,6 @@ #include #include #include -#include -#include #include "github.h" #include @@ -124,55 +122,75 @@ void GitHub::request(Method method, const QString &path, const QByteArray &data, const std::function &callback, bool relative) { - // make sure the timer is a child of this so it's deleted correctly and + // make sure the timer is owned by this so it's deleted correctly and // doesn't fire after the GitHub object is destroyed; this happens when // restarting MO by switching instances, for example QTimer *timer = new QTimer(this); - timer->setSingleShot(true); - timer->setInterval(30000); + timer->setInterval(10000); + QNetworkReply *reply = genReply(method, path, data, relative); - // remember this reply + // remember this reply so it can be deleted in the destructor if necessary m_replies.push_back(reply); - connect(reply, &QNetworkReply::finished, [this, reply, timer, method, data, callback]() { - QJsonDocument result = handleReply(reply); - QJsonObject object = result.object(); - timer->stop(); - if (object.value("http_status").toDouble() == 301.0) { - request(method, object.value("redirection").toString(), data, callback, - false); - } else { - callback(result); - } + Request req = {method, data, callback, timer, reply}; - deleteReply(reply); - }); + // finished + connect(reply, &QNetworkReply::finished, [this, req]{ onFinished(req); }); - connect(reply, - static_cast( - &QNetworkReply::error), - [this, reply, timer, callback](QNetworkReply::NetworkError error) { - qDebug("network error %d", error); - timer->stop(); - reply->disconnect(); - callback(QJsonDocument( - QJsonObject({{"network_error", reply->errorString()}}))); + // error + connect( + reply, qOverload(&QNetworkReply::error), + [this, req](auto&& error){ onError(req, error); }); - deleteReply(reply); - }); - - connect(timer, &QTimer::timeout, [this, reply]() { - qDebug("timeout"); - - // don't delete the reply, abort will fire the error() handler above - reply->abort(); - }); + // timeout + connect(timer, &QTimer::timeout, [this, req]{ onTimeout(req); }); timer->start(); } +void GitHub::onFinished(const Request& req) +{ + QJsonDocument result = handleReply(req.reply); + QJsonObject object = result.object(); + + req.timer->stop(); + + if (object.value("http_status").toInt() == 301) { + request( + req.method, object.value("redirection").toString(), + req.data, req.callback, false); + } else { + req.callback(result); + } + + deleteReply(req.reply); +} + +void GitHub::onError(const Request& req, QNetworkReply::NetworkError error) +{ + qDebug("network error %d", error); + + req.timer->stop(); + req.reply->disconnect(); + + QJsonObject root({{"network_error", req.reply->errorString()}}); + QJsonDocument doc(root); + + req.callback(doc); + + deleteReply(req.reply); +} + +void GitHub::onTimeout(const Request& req) +{ + qDebug("timeout"); + + // don't delete the reply, abort will fire the error() handler above + req.reply->abort(); +} + void GitHub::deleteReply(QNetworkReply* reply) { // remove from the list diff --git a/src/github.h b/src/github.h index 9faaca2..b8d5ffb 100644 --- a/src/github.h +++ b/src/github.h @@ -5,6 +5,8 @@ #include #include #include +#include +#include #include class GitHubException : public std::exception @@ -85,10 +87,23 @@ private: const QByteArray &data, bool relative); private: + struct Request + { + Method method = Method::GET; + QByteArray data; + std::function callback; + QTimer* timer = nullptr; + QNetworkReply* reply = nullptr; + }; + QNetworkAccessManager *m_AccessManager; // remember the replies that are in flight and delete them in the destructor std::vector m_replies; + void onFinished(const Request& req); + void onError(const Request& req, QNetworkReply::NetworkError error); + void onTimeout(const Request& req); + void deleteReply(QNetworkReply* reply); }; From ce23a98ae1c8e2436bad6359b154b5a3e8691552 Mon Sep 17 00:00:00 2001 From: isanae <14251494+isanae@users.noreply.github.com> Date: Fri, 12 Jul 2019 02:44:00 -0400 Subject: [PATCH 3/3] better error logging --- src/github.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/github.cpp b/src/github.cpp index 9bdc766..6e247f7 100644 --- a/src/github.cpp +++ b/src/github.cpp @@ -170,7 +170,13 @@ void GitHub::onFinished(const Request& req) void GitHub::onError(const Request& req, QNetworkReply::NetworkError error) { - qDebug("network error %d", error); + // the only way the request can be aborted is when there's a timeout, which + // already logs a message + if (error != QNetworkReply::OperationCanceledError) { + qCritical().noquote().nospace() + << "Github: request for " << req.reply->url().toString() << " failed, " + << req.reply->errorString() << " (" << error << ")"; + } req.timer->stop(); req.reply->disconnect(); @@ -185,7 +191,8 @@ void GitHub::onError(const Request& req, QNetworkReply::NetworkError error) void GitHub::onTimeout(const Request& req) { - qDebug("timeout"); + qCritical().noquote().nospace() + << "Github: request for " << req.reply->url().toString() << " timed out"; // don't delete the reply, abort will fire the error() handler above req.reply->abort();