Merge #838(kitsune): Expose job backoff strategies and make them conf…

…igurable
quotient-im · Nov 28, 2024 · e44d3a2 · e44d3a2
2 parents 688915f + bee9d44
commit e44d3a2
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 38 deletions.
diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp
@@ -32,7 +32,6 @@
 #include "events/encryptionevent.h"
 #include "jobs/downloadfilejob.h"
 #include "jobs/mediathumbnailjob.h"
-#include "jobs/syncjob.h"
 
 // moc needs fully defined deps, see https://www.qt.io/blog/whats-new-in-qmetatype-qvariant
 #include "moc_connection.cpp" // NOLINT(bugprone-suspicious-include)
@@ -191,7 +190,8 @@ void Connection::assumeIdentity(const QString& mxId, const QString& deviceId,
                         << ") is different from passed MXID (" << mxId << ")!";
                 return;
             case BaseJob::NetworkError:
-                emit networkError(job->errorString(), job->rawDataSample(), job->maxRetries(), -1);
+                QT_IGNORE_DEPRECATIONS(emit networkError(job->errorString(), job->rawDataSample(),
+                                                         job->maxRetries(), -1);)
                 return;
             default: emit loginError(job->errorString(), job->rawDataSample());
             }

diff --git a/Quotient/connection.h b/Quotient/connection.h
@@ -18,6 +18,7 @@
 
 #include "events/accountdataevents.h"
 #include "jobs/jobhandle.h"
+#include "jobs/syncjob.h"
 
 #include <QtCore/QDir>
 #include <QtCore/QObject>
@@ -40,8 +41,6 @@ class RoomEvent;
 
 class GetVersionsJob;
 class GetCapabilitiesJob;
-class SyncJob;
-class SyncData;
 class RoomMessagesJob;
 class PostReceiptJob;
 class ForgetRoomJob;
@@ -692,7 +691,7 @@ public Q_SLOTS:
     QFuture<void> logout();
 
     void sync(int timeout = -1);
-    void syncLoop(int timeout = 30000);
+    void syncLoop(int timeout = SyncJob::defaultTimeoutMillis);
 
     void stopSync();
 

diff --git a/Quotient/jobs/basejob.cpp b/Quotient/jobs/basejob.cpp
@@ -20,8 +20,7 @@
 #include <ranges>
 
 using namespace Quotient;
-using std::chrono::seconds, std::chrono::milliseconds;
-using namespace std::chrono_literals;
+using namespace std::chrono;
 
 BaseJob::StatusCode BaseJob::Status::fromHttpCode(int httpCode)
 {
@@ -67,11 +66,6 @@ QDebug BaseJob::Status::dumpToLog(QDebug dbg) const
 
 class Q_DECL_HIDDEN BaseJob::Private {
 public:
-    struct JobTimeoutConfig {
-        seconds jobTimeout;
-        seconds nextRetryInterval;
-    };
-
     // Using an idiom from clang-tidy:
     // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
     Private(HttpVerb v, QByteArray endpoint, const QUrlQuery& q,
@@ -147,16 +141,10 @@ class Q_DECL_HIDDEN BaseJob::Private {
     QTimer timer;
     QTimer retryTimer;
 
-    static constexpr auto errorStrategy = std::to_array<const JobTimeoutConfig>(
-        { { 30s, 2s }, { 60s, 5s }, { 150s, 30s } });
-    int maxRetries = int(errorStrategy.size());
-    int retriesTaken = 0;
+    static inline JobBackoffStrategy defaultBackoffStrategy{ { 45s, 90s, 150s }, { 2s, 5s } };
 
-    [[nodiscard]] const JobTimeoutConfig& getCurrentTimeoutConfig() const
-    {
-        return errorStrategy[std::min(size_t(retriesTaken),
-                                      errorStrategy.size() - 1)];
-    }
+    JobBackoffStrategy backoffStrategy = defaultBackoffStrategy;
+    int retriesTaken = 0;
 
     [[nodiscard]] QString dumpRequest() const
     {
@@ -599,12 +587,11 @@ void BaseJob::finishJob()
     case NetworkError:
     case IncorrectResponse:
     case Timeout:
-        if (d->retriesTaken < d->maxRetries) {
+        if (!d->backoffStrategy.maxRetries || d->retriesTaken < *d->backoffStrategy.maxRetries) {
             // TODO: The whole retrying thing should be put to
             // Connection(Manager) otherwise independently retrying jobs make a
             // bit of notification storm towards the UI.
-            const seconds retryIn = error() == Timeout ? 0s
-                                                       : getNextRetryInterval();
+            const auto retryIn = error() == Timeout ? 0s : getNextRetryInterval();
             ++d->retriesTaken;
             qCWarning(d->logCat).nospace()
                 << this << ": retry #" << d->retriesTaken << " in "
@@ -633,19 +620,25 @@ void BaseJob::finishJob()
     deleteLater();
 }
 
-seconds BaseJob::getCurrentTimeout() const
+inline auto atOrLast(const auto& values, auto index)
 {
-    return d->getCurrentTimeoutConfig().jobTimeout;
+    QUO_CHECK(!values.empty());
+    return index < values.size() ? values[index] : values.back();
+}
+
+JobBackoffStrategy::duration_t BaseJob::getCurrentTimeout() const
+{
+    return atOrLast(d->backoffStrategy.jobTimeouts, d->retriesTaken);
 }
 
 BaseJob::duration_ms_t BaseJob::getCurrentTimeoutMs() const
 {
     return milliseconds(getCurrentTimeout()).count();
 }
 
-seconds BaseJob::getNextRetryInterval() const
+JobBackoffStrategy::duration_t BaseJob::getNextRetryInterval() const
 {
-    return d->getCurrentTimeoutConfig().nextRetryInterval;
+    return atOrLast(d->backoffStrategy.nextRetryIntervals, d->retriesTaken);
 }
 
 BaseJob::duration_ms_t BaseJob::getNextRetryMs() const
@@ -664,11 +657,33 @@ BaseJob::duration_ms_t BaseJob::millisToRetry() const
     return timeToRetry().count();
 }
 
-int BaseJob::maxRetries() const { return d->maxRetries; }
+int BaseJob::maxRetries() const
+{
+    return d->backoffStrategy.maxRetries ? static_cast<int>(*d->backoffStrategy.maxRetries)
+                                         : std::numeric_limits<int>::max();
+}
 
 void BaseJob::setMaxRetries(int newMaxRetries)
 {
-    d->maxRetries = newMaxRetries;
+    d->backoffStrategy.maxRetries = newMaxRetries;
+}
+
+JobBackoffStrategy BaseJob::currentBackoffStrategy() const { return d->backoffStrategy; }
+
+void BaseJob::setBackoffStrategy(JobBackoffStrategy strategy)
+{
+    QUO_CHECK(!strategy.jobTimeouts.empty());
+    QUO_CHECK(!strategy.nextRetryIntervals.empty());
+    d->backoffStrategy = std::move(strategy);
+}
+
+JobBackoffStrategy BaseJob::defaultBackoffStrategy() { return Private::defaultBackoffStrategy; }
+
+void BaseJob::setDefaultBackoffStrategy(JobBackoffStrategy defaultStrategy)
+{
+    QUO_CHECK(!defaultStrategy.jobTimeouts.empty());
+    QUO_CHECK(!defaultStrategy.nextRetryIntervals.empty());
+    Private::defaultBackoffStrategy = std::move(defaultStrategy);
 }
 
 BaseJob::Status BaseJob::status() const { return d->status; }

diff --git a/Quotient/jobs/basejob.h b/Quotient/jobs/basejob.h
@@ -23,6 +23,14 @@ class ConnectionData;
 
 enum class HttpVerb { Get, Put, Post, Delete };
 
+struct JobBackoffStrategy {
+    using duration_t = std::chrono::seconds;
+    QVector<duration_t> jobTimeouts;
+    QVector<duration_t> nextRetryIntervals;
+    //! How many times a network request should be tried; std::nullopt means keep trying forever
+    std::optional<decltype(jobTimeouts)::size_type> maxRetries = jobTimeouts.size();
+};
+
 class QUOTIENT_API BaseJob : public QObject {
     Q_OBJECT
     Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT)
@@ -206,14 +214,28 @@ class QUOTIENT_API BaseJob : public QObject {
     //! A URL to help/clarify the error, if provided by the server
     QUrl errorUrl() const;
 
+    [[deprecated("Use currentBackoffStrategy().maxRetries instead")]]
     int maxRetries() const;
+    [[deprecated("Use setBackoffStrategy() instead")]]
     void setMaxRetries(int newMaxRetries);
 
+    //! Get the back-off strategy for this job instance
+    JobBackoffStrategy currentBackoffStrategy() const;
+    //! Set the back-off strategy for this specific job instance
+    void setBackoffStrategy(JobBackoffStrategy strategy);
+
+    //! Get the default back-off strategy used for any newly created job
+    static JobBackoffStrategy defaultBackoffStrategy();
+    //! \brief Set the default back-off strategy to use for any newly created job
+    //! \note This back-off strategy does not apply to SyncJob; it has a separate default but you
+    //!       can still override it per job instance after creating it
+    static void setDefaultBackoffStrategy(JobBackoffStrategy defaultStrategy);
+
     using duration_ms_t = std::chrono::milliseconds::rep; // normally int64_t
 
-    std::chrono::seconds getCurrentTimeout() const;
+    JobBackoffStrategy::duration_t getCurrentTimeout() const;
     Q_INVOKABLE Quotient::BaseJob::duration_ms_t getCurrentTimeoutMs() const;
-    std::chrono::seconds getNextRetryInterval() const;
+    JobBackoffStrategy::duration_t getNextRetryInterval() const;
     Q_INVOKABLE Quotient::BaseJob::duration_ms_t getNextRetryMs() const;
     std::chrono::milliseconds timeToRetry() const;
     Q_INVOKABLE Quotient::BaseJob::duration_ms_t millisToRetry() const;

diff --git a/Quotient/jobs/syncjob.cpp b/Quotient/jobs/syncjob.cpp
@@ -16,12 +16,17 @@ SyncJob::SyncJob(const QString& since, const QString& filter, int timeout, const
     QUrlQuery query;
     addParam<IfNotEmpty>(query, u"filter"_s, filter);
     addParam<IfNotEmpty>(query, u"set_presence"_s, presence);
-    if (timeout >= 0)
+    using namespace std::chrono_literals;
+    // Add time for the request roundtrip on top of the server-side timeout specified above
+    JobBackoffStrategy backoffStrategy { { defaultTimeout + 10s }, { 2s, 5s, 15s }, std::nullopt };
+    if (timeout >= 0) {
         query.addQueryItem(u"timeout"_s, QString::number(timeout));
+        backoffStrategy.jobTimeouts = { std::chrono::seconds(timeout / 1000 + 10) };
+    } else
+        backoffStrategy.jobTimeouts = { std::chrono::seconds::max() };
+    setBackoffStrategy(std::move(backoffStrategy));
     addParam<IfNotEmpty>(query, u"since"_s, since);
     setRequestQuery(query);
-
-    setMaxRetries(std::numeric_limits<int>::max());
 }
 
 SyncJob::SyncJob(const QString& since, const Filter& filter, int timeout,

diff --git a/Quotient/jobs/syncjob.h b/Quotient/jobs/syncjob.h
@@ -10,10 +10,14 @@
 namespace Quotient {
 class QUOTIENT_API SyncJob : public BaseJob {
 public:
+    static constexpr auto defaultTimeout = std::chrono::seconds(30);
+    static constexpr auto defaultTimeoutMillis =
+        std::chrono::milliseconds(defaultTimeout).count();
+
     explicit SyncJob(const QString& since = {}, const QString& filter = {},
-                     int timeout = -1, const QString& presence = {});
-    explicit SyncJob(const QString& since, const Filter& filter,
-                     int timeout = -1, const QString& presence = {});
+                     int timeout = defaultTimeoutMillis, const QString& presence = {});
+    explicit SyncJob(const QString& since, const Filter& filter, int timeout = defaultTimeoutMillis,
+                     const QString& presence = {});
 
     SyncData takeData() { return std::move(d); }