Merge pull request #327 from vranki/bugfix/forget_rooms_really2

Ignore some errors on leaving rooms, add new error enum. Fixes #307

3 files changed, 100 insertions, 64 deletions

diff --git a/lib/connection.cpp b/lib/connection.cpp
index a7eae30f..4c068b8f 100644
--- a/lib/connection.cpp
+++ b/lib/connection.cpp
@@ -110,6 +110,7 @@ class Connection::Private
 
         void connectWithToken(const QString& user, const QString& accessToken,
                               const QString& deviceId);
+        void removeRoom(const QString& roomId);
 
         template <typename EventT>
         EventT* unpackAccountData() const
@@ -790,29 +791,36 @@ ForgetRoomJob* Connection::forgetRoom(const QString& id)
     if (room && room->joinState() != JoinState::Leave)
     {
         auto leaveJob = room->leaveRoom();
-        connect(leaveJob, &BaseJob::success, this, [this, forgetJob, room] {
-            forgetJob->start(connectionData());
-            // If the matching /sync response hasn't arrived yet, mark the room
-            // for explicit deletion
-            if (room->joinState() != JoinState::Leave)
-                d->roomIdsToForget.push_back(room->id());
-        });
+        connect(leaveJob, &BaseJob::result, this,
+                [this, leaveJob, forgetJob, room] {
+                    if (leaveJob->error() == BaseJob::Success
+                        || leaveJob->error() == BaseJob::NotFoundError)
+                    {
+                        forgetJob->start(connectionData());
+                        // If the matching /sync response hasn't arrived yet,
+                        // mark the room for explicit deletion
+                        if (room->joinState() != JoinState::Leave)
+                            d->roomIdsToForget.push_back(room->id());
+                    } else {
+                        qCWarning(MAIN).nospace()
+                            << "Error leaving room " << room->objectName()
+                            << ": " << leaveJob->errorString();
+                        forgetJob->abandon();
+                    }
+                });
         connect(leaveJob, &BaseJob::failure, forgetJob, &BaseJob::abandon);
     }
     else
         forgetJob->start(connectionData());
-    connect(forgetJob, &BaseJob::success, this, [this, id]
+    connect(forgetJob, &BaseJob::result, this, [this, id, forgetJob]
     {
-        // Delete whatever instances of the room are still in the map.
-        for (auto f: {false, true})
-            if (auto r = d->roomMap.take({ id, f }))
-            {
-                qCDebug(MAIN) << "Room" << r->objectName()
-                              << "in state" << toCString(r->joinState())
-                              << "will be deleted";
-                emit r->beforeDestruction(r);
-                r->deleteLater();
-            }
+        // Leave room in case of success, or room not known by server
+        if(forgetJob->error() == BaseJob::Success
+                || forgetJob->error() == BaseJob::NotFoundError)
+            d->removeRoom(id); // Delete the room from roomMap
+        else
+            qCWarning(MAIN).nospace() << "Error forgetting room " << id << ": "
+                                      << forgetJob->errorString();
     });
     return forgetJob;
 }
@@ -1056,6 +1064,20 @@ Connection::DirectChatsMap Connection::directChats() const
     return d->directChats;
 }
 
+// Removes room with given id from roomMap
+void Connection::Private::removeRoom(const QString& roomId)
+{
+    for (auto f: {false, true})
+        if (auto r = roomMap.take({ roomId, f }))
+        {
+            qCDebug(MAIN) << "Room" << r->objectName()
+                          << "in state" << toCString(r->joinState())
+                          << "will be deleted";
+            emit r->beforeDestruction(r);
+            r->deleteLater();
+        }
+}
+
 void Connection::addToDirectChats(const Room* room, User* user)
 {
     Q_ASSERT(room != nullptr && user != nullptr);
diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp
index 34fc0f57..6ad8d971 100644
--- a/lib/jobs/basejob.cpp
+++ b/lib/jobs/basejob.cpp
@@ -289,56 +289,23 @@ void BaseJob::gotReply()
     if (status().good())
         setStatus(parseReply(d->reply.data()));
     else {
-        // FIXME: Factor out to smth like BaseJob::handleError()
         d->rawResponse = d->reply->readAll();
         const auto jsonBody =
-                d->reply->rawHeader("Content-Type") == "application/json";
+            d->reply->rawHeader("Content-Type") == "application/json";
         qCDebug(d->logCat).noquote()
-                << "Error body (truncated if long):" << d->rawResponse.left(500);
+            << "Error body (truncated if long):" << d->rawResponse.left(500);
         if (jsonBody)
-        {
-            auto json = QJsonDocument::fromJson(d->rawResponse).object();
-            const auto errCode = json.value("errcode"_ls).toString();
-            if (error() == TooManyRequestsError ||
-                    errCode == "M_LIMIT_EXCEEDED")
-            {
-                QString msg = tr("Too many requests");
-                auto retryInterval = json.value("retry_after_ms"_ls).toInt(-1);
-                if (retryInterval != -1)
-                    msg += tr(", next retry advised after %1 ms")
-                            .arg(retryInterval);
-                else // We still have to figure some reasonable interval
-                    retryInterval = getNextRetryInterval();
-
-                setStatus(TooManyRequestsError, msg);
-
-                // Shortcut to retry instead of executing finishJob()
-                stop();
-                qCWarning(d->logCat)
-                        << this << "will retry in" << retryInterval << "ms";
-                d->retryTimer.start(retryInterval);
-                emit retryScheduled(d->retriesTaken, retryInterval);
-                return;
-            }
-            if (errCode == "M_CONSENT_NOT_GIVEN")
-            {
-                d->status.code = UserConsentRequiredError;
-                d->errorUrl = json.value("consent_uri"_ls).toString();
-            }
-            else if (errCode == "M_UNSUPPORTED_ROOM_VERSION" ||
-                     errCode == "M_INCOMPATIBLE_ROOM_VERSION")
-            {
-                d->status.code = UnsupportedRoomVersionError;
-                if (json.contains("room_version"))
-                    d->status.message =
-                        tr("Requested room version: %1")
-                        .arg(json.value("room_version").toString());
-            } else if (!json.isEmpty()) // Not localisable on the client side
-                setStatus(d->status.code, json.value("error"_ls).toString());
-        }
+            setStatus(
+                parseError(d->reply.data(),
+                           QJsonDocument::fromJson(d->rawResponse).object()));
     }
 
-    finishJob();
+    if (error() != TooManyRequestsError)
+        finishJob();
+    else {
+        stop();
+        emit retryScheduled(d->retriesTaken, d->retryTimer.interval());
+    }
 }
 
 bool checkContentType(const QByteArray& type, const QByteArrayList& patterns)
@@ -442,8 +409,46 @@ BaseJob::Status BaseJob::parseJson(const QJsonDocument&)
     return Success;
 }
 
+BaseJob::Status BaseJob::parseError(QNetworkReply* reply,
+                                    const QJsonObject& errorJson)
+{
+    const auto errCode = errorJson.value("errcode"_ls).toString();
+    if (error() == TooManyRequestsError || errCode == "M_LIMIT_EXCEEDED") {
+        QString msg = tr("Too many requests");
+        auto retryInterval = errorJson.value("retry_after_ms"_ls).toInt(-1);
+        if (retryInterval != -1)
+            msg += tr(", next retry advised after %1 ms").arg(retryInterval);
+        else // We still have to figure some reasonable interval
+            retryInterval = getNextRetryInterval();
+
+        qCWarning(d->logCat) << this << "will retry in" << retryInterval << "ms";
+        d->retryTimer.start(retryInterval);
+
+        return { TooManyRequestsError, msg };
+    }
+    if (errCode == "M_CONSENT_NOT_GIVEN") {
+        d->errorUrl = errorJson.value("consent_uri"_ls).toString();
+        return { UserConsentRequiredError };
+    }
+    if (errCode == "M_UNSUPPORTED_ROOM_VERSION"
+        || errCode == "M_INCOMPATIBLE_ROOM_VERSION")
+        return { UnsupportedRoomVersionError,
+                 errorJson.contains("room_version"_ls)
+                     ? tr("Requested room version: %1")
+                           .arg(errorJson.value("room_version"_ls).toString())
+                     : errorJson.value("error"_ls).toString() };
+
+    // Not localisable on the client side
+    if (errorJson.contains("error"_ls))
+        d->status.message = errorJson.value("error"_ls).toString();
+
+    return d->status;
+}
+
 void BaseJob::stop()
 {
+    // This method is used to semi-finalise the job before retrying; so
+    // stop the timeout timer but keep the retry timer running.
     d->timer.stop();
     if (d->reply)
     {
diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h
index 4ee63adb..d435749a 100644
--- a/lib/jobs/basejob.h
+++ b/lib/jobs/basejob.h
@@ -310,7 +310,7 @@ namespace QMatrixClient
              * Processes the reply. By default, parses the reply into
              * a QJsonDocument and calls parseJson() if it's a valid JSON.
              *
-             * @param reply raw contents of a HTTP reply from the server (without headers)
+             * @param reply raw contents of a HTTP reply from the server
              *
              * @see gotReply, parseJson
              */
@@ -318,7 +318,7 @@ namespace QMatrixClient
 
             /**
              * Processes the JSON document received from the Matrix server.
-             * By default returns succesful status without analysing the JSON.
+             * By default returns successful status without analysing the JSON.
              *
              * @param json valid JSON document received from the server
              *
@@ -326,6 +326,15 @@ namespace QMatrixClient
              */
             virtual Status parseJson(const QJsonDocument&);
 
+            /**
+             * Processes the reply in case of unsuccessful HTTP code.
+             * The body is already loaded from the reply object to errorJson.
+             * @param reply the HTTP reply from the server
+             * @param errorJson the JSON payload describing the error
+             */
+            virtual Status parseError(QNetworkReply* reply,
+                                      const QJsonObject& errorJson);
+
             void setStatus(Status s);
             void setStatus(int code, QString message);