Merge pull request #4 from isanae/reply-timeout-crash

Reply timeout crash

src/github.cpp

@@ -1,8 +1,6 @@
 #include <QJsonDocument>
 #include <QEventLoop>
 #include <QNetworkRequest>
-#include <QNetworkReply>
-#include <QTimer>
 #include "github.h"
 
 #include <QThread>
@@ -21,6 +19,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,39 +122,90 @@ void GitHub::request(Method method, const QString &path, const QByteArray &data,
                      const std::function<void(const QJsonDocument &)> &callback,
                      bool relative)
 {
-  QTimer *timer = new QTimer();
+  // 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);
 
-  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);
-    }
-    reply->deleteLater();
-  });
+  // remember this reply so it can be deleted in the destructor if necessary
+  m_replies.push_back(reply);
 
-  connect(reply,
-          static_cast<void (QNetworkReply::*)(QNetworkReply::NetworkError)>(
-              &QNetworkReply::error),
-          [reply, timer, callback](QNetworkReply::NetworkError error) {
-            qDebug("network error %d", error);
-            timer->stop();
-            reply->disconnect();
-            callback(QJsonDocument(
-                QJsonObject({{"network_error", reply->errorString()}})));
-            reply->deleteLater();
-          });
+  Request req = {method, data, callback, timer, reply};
+
+  // finished
+  connect(reply, &QNetworkReply::finished, [this, req]{ onFinished(req); });
+
+  // error
+  connect(
+    reply, qOverload<QNetworkReply::NetworkError>(&QNetworkReply::error),
+    [this, req](auto&& error){ onError(req, error); });
+
+  // timeout
+  connect(timer, &QTimer::timeout, [this, req]{ onTimeout(req); });
 
-  connect(timer, &QTimer::timeout, [reply]() {
-    qDebug("timeout");
-    reply->abort();
-  });
   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)
+{
+  // 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();
+
+  QJsonObject root({{"network_error", req.reply->errorString()}});
+  QJsonDocument doc(root);
+
+  req.callback(doc);
+
+  deleteReply(req.reply);
+}
+
+void GitHub::onTimeout(const Request& req)
+{
+  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();
+}
+
+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();
+}

src/github.h

@@ -5,6 +5,8 @@
 #include <QJsonDocument>
 #include <QJsonObject>
 #include <QJsonArray>
+#include <QTimer>
+#include <QNetworkReply>
 #include <functional>
 
 class GitHubException : public std::exception
@@ -67,6 +69,7 @@ public:
 
 public:
   GitHub(const char *clientId = nullptr);
+  ~GitHub();
 
   QJsonArray releases(const Repository &repo);
   void releases(const Repository &repo,
@@ -84,5 +87,23 @@ private:
                           const QByteArray &data, bool relative);
 
 private:
+  struct Request
+  {
+    Method method = Method::GET;
+    QByteArray data;
+    std::function<void (const QJsonDocument &)> 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<QNetworkReply*> m_replies;
+
+  void onFinished(const Request& req);
+  void onError(const Request& req, QNetworkReply::NetworkError error);
+  void onTimeout(const Request& req);
+
+  void deleteReply(QNetworkReply* reply);
 };