Fix subtile bugs on reconnect

* Ensure previous long polling requests for events are aborted
  before connecting again
* Ensure results from previous requests are always discarded
  after aborting to reconnect

connector/syncthingconnection.cpp

@@ -66,7 +66,7 @@ SyncthingConnection::SyncthingConnection(const QString &syncthingUrl, const QByt
     , m_apiKey(apiKey)
     , m_status(SyncthingStatus::Disconnected)
     , m_keepPolling(false)
-    , m_reconnecting(false)
+    , m_abortingToReconnect(false)
     , m_requestCompletion(false)
     , m_lastEventId(0)
     , m_lastDiskEventId(0)
@@ -201,7 +201,7 @@ void SyncthingConnection::connect()
     }
 
     // reset status
-    m_reconnecting = m_hasConfig = m_hasStatus = m_hasEvents = m_hasDiskEvents = false;
+    m_abortingToReconnect = m_hasConfig = m_hasStatus = m_hasEvents = m_hasDiskEvents = false;
 
     // check configuration
     if (m_apiKey.isEmpty() || m_syncthingUrl.isEmpty()) {
@@ -241,7 +241,7 @@ void SyncthingConnection::connectLater(int milliSeconds)
  */
 void SyncthingConnection::disconnect()
 {
-    m_reconnecting = m_keepPolling = false;
+    m_abortingToReconnect = m_keepPolling = false;
     m_autoReconnectTries = 0;
     abortAllRequests();
 }
@@ -296,16 +296,22 @@ void SyncthingConnection::abortAllRequests()
  */
 void SyncthingConnection::reconnect()
 {
+    // reset reconnect timer
     m_autoReconnectTimer.stop();
     m_autoReconnectTries = 0;
-    // reconnect right now if not connected and no pending requests
+
-    if (!isConnected() && !hasPendingRequests()) {
+    // reset variables to track connection progress
+    // note: especially resetting events is important as it influences the subsequent hasPendingRequests() call
+    m_hasConfig = m_hasStatus = m_hasEvents = m_hasDiskEvents = false;
+
+    // reconnect right now if no pending requests to be aborted
+    if (!hasPendingRequests()) {
         continueReconnecting();
         return;
     }
+
     // abort pending requests before connecting again
-    m_keepPolling = m_reconnecting = true;
+    m_keepPolling = m_abortingToReconnect = true;
-    m_hasConfig = m_hasStatus = m_hasEvents = m_hasDiskEvents = false;
     abortAllRequests();
 }
 
@@ -332,17 +338,15 @@ void SyncthingConnection::reconnectLater(int milliSeconds)
  */
 void SyncthingConnection::continueReconnecting()
 {
-    // postpone if there are still pending requests
+    // notify that we're about to invalidate the configuration if not already invalidated anyways
-    if (hasPendingRequests()) {
+    const auto isConfigInvalidated = m_rawConfig.isEmpty();
-        return;
+    if (!isConfigInvalidated) {
+        emit newConfig(m_rawConfig = QJsonObject());
     }
 
-    // invalidate configuration
-    emit newConfig(QJsonObject());
-
     // cleanup information from previous connection
     m_keepPolling = true;
-    m_reconnecting = false;
+    m_abortingToReconnect = false;
     m_lastEventId = 0;
     m_lastDiskEventId = 0;
     m_configDir.clear();
@@ -367,6 +371,11 @@ void SyncthingConnection::continueReconnecting()
     m_lastFileDeleted = false;
     m_syncthingVersion.clear();
 
+    // notify that the configuration has been invalidated
+    if (!isConfigInvalidated) {
+        emit newConfigApplied();
+    }
+
     if (m_apiKey.isEmpty() || m_syncthingUrl.isEmpty()) {
         emit error(tr("Connection configuration is insufficient."), SyncthingErrorCategory::OverallConnection, QNetworkReply::NoError);
         return;
@@ -391,6 +400,8 @@ void SyncthingConnection::concludeReadingConfigAndStatus()
 
     readDevs(m_rawConfig.value(QLatin1String("devices")).toArray());
     readDirs(m_rawConfig.value(QLatin1String("folders")).toArray());
+    emit newConfigApplied();
+
     continueConnecting();
 }
 
@@ -863,10 +874,14 @@ void SyncthingConnection::handleFatalConnectionError()
  */
 void SyncthingConnection::handleAdditionalRequestCanceled()
 {
-    if (m_reconnecting) {
+    // postpone handling if there are still other requests pending
+    if (hasPendingRequests()) {
+        return;
+    }
+    if (m_abortingToReconnect) {
         // if reconnection flag is set, instantly etstablish a new connection ...
         continueReconnecting();
-    } else if (!hasPendingRequests()) {
+    } else {
         // ... otherwise declare we're disconnected if that was the last pending request
         setStatus(SyncthingStatus::Disconnected);
     }

connector/syncthingconnection.h

@@ -112,6 +112,7 @@ public:
     static QString statusText(SyncthingStatus status);
     bool isConnected() const;
     bool hasPendingRequests() const;
+    bool hasPendingRequestsIncludingEvents() const;
     bool hasUnreadNotifications() const;
     bool hasOutOfSyncDirs() const;
 
@@ -213,6 +214,7 @@ Q_SIGNALS:
     void newConfig(const QJsonObject &rawConfig);
     void newDirs(const std::vector<SyncthingDir> &dirs);
     void newDevices(const std::vector<SyncthingDev> &devs);
+    void newConfigApplied();
     void newEvents(const QJsonArray &events);
     void dirStatusChanged(const SyncthingDir &dir, int index);
     void devStatusChanged(const SyncthingDev &dev, int index);
@@ -305,6 +307,9 @@ private:
     QNetworkRequest prepareRequest(const QString &path, const QUrlQuery &query, bool rest = true);
     QNetworkReply *requestData(const QString &path, const QUrlQuery &query, bool rest = true);
     QNetworkReply *postData(const QString &path, const QUrlQuery &query, const QByteArray &data = QByteArray());
+    QNetworkReply *prepareReply();
+    QNetworkReply *prepareReply(QNetworkReply *&expectedReply);
+    QNetworkReply *prepareReply(QList<QNetworkReply *> &expectedReplies);
     bool pauseResumeDevice(const QStringList &devIds, bool paused);
     bool pauseResumeDirectory(const QStringList &dirIds, bool paused);
     SyncthingDir *addDirInfo(std::vector<SyncthingDir> &dirs, const QString &dirId);
@@ -316,7 +321,7 @@ private:
     QString m_password;
     SyncthingStatus m_status;
     bool m_keepPolling;
-    bool m_reconnecting;
+    bool m_abortingToReconnect;
     bool m_requestCompletion;
     int m_lastEventId;
     int m_lastDiskEventId;

connector/syncthingconnection_requests.cpp

@@ -81,6 +81,65 @@ QNetworkReply *SyncthingConnection::postData(const QString &path, const QUrlQuer
     return reply;
 }
 
+/*!
+ * \brief Prepares the current reply.
+ */
+QNetworkReply *SyncthingConnection::prepareReply()
+{
+    auto *const reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+
+    // skip further processing if aborting to reconnect
+    if (m_abortingToReconnect) {
+        handleAdditionalRequestCanceled();
+        return nullptr;
+    }
+
+    return reply;
+}
+
+/*!
+ * \brief Prepares the current reply.
+ */
+QNetworkReply *SyncthingConnection::prepareReply(QNetworkReply *&expectedReply)
+{
+    auto *const reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+
+    // unset the expected reply so it is no longer considered pending
+    if (reply == expectedReply) {
+        expectedReply = nullptr;
+    }
+
+    // skip further processing if aborting to reconnect
+    if (m_abortingToReconnect) {
+        handleAdditionalRequestCanceled();
+        return nullptr;
+    }
+
+    return reply;
+}
+
+/*!
+ * \brief Prepares the current reply.
+ */
+QNetworkReply *SyncthingConnection::prepareReply(QList<QNetworkReply *> &expectedReplies)
+{
+    auto *const reply = static_cast<QNetworkReply *>(sender());
+    reply->deleteLater();
+
+    // unset the expected reply so it is no longer considered pending
+    expectedReplies.removeAll(reply);
+
+    // skip further processing if aborting to reconnect
+    if (m_abortingToReconnect) {
+        handleAdditionalRequestCanceled();
+        return nullptr;
+    }
+
+    return reply;
+}
+
 // pause/resume devices
 
 /*!
@@ -159,8 +218,11 @@ bool SyncthingConnection::pauseResumeDevice(const QStringList &devIds, bool paus
  */
 void SyncthingConnection::readDevPauseResume()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
+
     switch (reply->error()) {
     case QNetworkReply::NoError: {
         const QStringList devIds(reply->property("devIds").toStringList());
@@ -260,8 +322,11 @@ bool SyncthingConnection::pauseResumeDirectory(const QStringList &dirIds, bool p
 
 void SyncthingConnection::readDirPauseResume()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
+
     switch (reply->error()) {
     case QNetworkReply::NoError: {
         const QStringList dirIds(reply->property("dirIds").toStringList());
@@ -326,8 +391,11 @@ void SyncthingConnection::rescan(const QString &dirId, const QString &relpath)
  */
 void SyncthingConnection::readRescan()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
+
     switch (reply->error()) {
     case QNetworkReply::NoError:
         emit rescanTriggered(reply->property("dirId").toString());
@@ -354,8 +422,11 @@ void SyncthingConnection::restart()
  */
 void SyncthingConnection::readRestart()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
+
     switch (reply->error()) {
     case QNetworkReply::NoError:
         emit restartTriggered();
@@ -380,8 +451,11 @@ void SyncthingConnection::shutdown()
  */
 void SyncthingConnection::readShutdown()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
+
     switch (reply->error()) {
     case QNetworkReply::NoError:
         emit shutdownTriggered();
@@ -409,8 +483,10 @@ void SyncthingConnection::requestClearingErrors()
  */
 void SyncthingConnection::readClearingErrors()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
 
     switch (reply->error()) {
     case QNetworkReply::NoError:
@@ -441,10 +517,9 @@ void SyncthingConnection::requestConfig()
  */
 void SyncthingConnection::readConfig()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_configReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_configReply) {
+        return;
-        m_configReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -469,6 +544,7 @@ void SyncthingConnection::readConfig()
 
         readDevs(m_rawConfig.value(QLatin1String("devices")).toArray());
         readDirs(m_rawConfig.value(QLatin1String("folders")).toArray());
+        emit newConfigApplied();
         break;
     }
     case QNetworkReply::OperationCanceledError:
@@ -580,10 +656,9 @@ void SyncthingConnection::requestStatus()
  */
 void SyncthingConnection::readStatus()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_statusReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_statusReply) {
+        return;
-        m_statusReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -647,10 +722,9 @@ void SyncthingConnection::requestConnections()
  */
 void SyncthingConnection::readConnections()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_connectionsReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_connectionsReply) {
+        return;
-        m_connectionsReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -757,10 +831,9 @@ void SyncthingConnection::requestErrors()
  */
 void SyncthingConnection::readErrors()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_errorsReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_errorsReply) {
+        return;
-        m_errorsReply = nullptr;
     }
 
     // ignore any errors occured before connecting
@@ -826,10 +899,9 @@ void SyncthingConnection::requestDirStatistics()
  */
 void SyncthingConnection::readDirStatistics()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_dirStatsReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_dirStatsReply) {
+        return;
-        m_dirStatsReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -913,9 +985,10 @@ void SyncthingConnection::requestDirStatus(const QString &dirId)
  */
 void SyncthingConnection::readDirStatus()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_otherReplies);
-    reply->deleteLater();
+    if (!reply) {
-    m_otherReplies.removeAll(reply);
+        return;
+    }
 
     switch (reply->error()) {
     case QNetworkReply::NoError: {
@@ -975,8 +1048,10 @@ void SyncthingConnection::requestDirPullErrors(const QString &dirId, int page, i
  */
 void SyncthingConnection::readDirPullErrors()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
 
     // determine relevant dir
     int index;
@@ -1028,11 +1103,12 @@ void SyncthingConnection::requestCompletion(const QString &devId, const QString
  */
 void SyncthingConnection::readCompletion()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_otherReplies);
+    if (!reply) {
+        return;
+    }
     const auto devId(reply->property("devId").toString());
     const auto dirId(reply->property("dirId").toString());
-    reply->deleteLater();
-    m_otherReplies.removeAll(reply);
 
     switch (reply->error()) {
     case QNetworkReply::NoError: {
@@ -1084,10 +1160,9 @@ void SyncthingConnection::requestDeviceStatistics()
  */
 void SyncthingConnection::readDeviceStatistics()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_devStatsReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_devStatsReply) {
+        return;
-        m_devStatsReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -1145,10 +1220,9 @@ void SyncthingConnection::requestVersion()
  */
 void SyncthingConnection::readVersion()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_versionReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_versionReply) {
+        return;
-        m_versionReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -1196,8 +1270,10 @@ void SyncthingConnection::requestQrCode(const QString &text)
  */
 void SyncthingConnection::readQrCode()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply();
-    reply->deleteLater();
+    if (!reply) {
+        return;
+    }
 
     switch (reply->error()) {
     case QNetworkReply::NoError:
@@ -1229,10 +1305,9 @@ void SyncthingConnection::requestLog()
  */
 void SyncthingConnection::readLog()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_logReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_logReply) {
+        return;
-        m_logReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -1440,10 +1515,9 @@ void SyncthingConnection::requestEvents()
  */
 void SyncthingConnection::readEvents()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_eventsReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_eventsReply) {
+        return;
-        m_eventsReply = nullptr;
     }
 
     switch (reply->error()) {
@@ -1975,10 +2049,9 @@ void SyncthingConnection::requestDiskEvents(int limit)
  */
 void SyncthingConnection::readDiskEvents()
 {
-    auto *const reply = static_cast<QNetworkReply *>(sender());
+    auto *const reply = prepareReply(m_diskEventsReply);
-    reply->deleteLater();
+    if (!reply) {
-    if (reply == m_diskEventsReply) {
+        return;
-        m_diskEventsReply = nullptr;
     }
 
     switch (reply->error()) {

connector/tests/connectiontests.cpp

@@ -479,8 +479,9 @@ void ConnectionTests::checkDirectories() const
 
 void ConnectionTests::testReconnecting()
 {
-    cerr << "\n - Reconnecting ..." << endl;
+    cerr << "\n - Reconnecting ...\n";
     waitForConnection(defaultReconnect(), 1000, connectionSignal(&SyncthingConnection::statusChanged));
+    cerr << "\n - Waiting for dirs/devs after reconnect ...\n";
     waitForAllDirsAndDevsReady(true);
     CPPUNIT_ASSERT_EQUAL_MESSAGE("connected again", QStringLiteral("connected, paused"), m_connection.statusText());
 }