diff --git a/Quotient/connection.cpp b/Quotient/connection.cpp index 33335dfef..3c19e1598 100644 --- 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 index f515aea63..11a5e196b 100644 --- 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 #include @@ -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 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 index 912ebf354..a5fc93d31 100644 --- a/Quotient/jobs/basejob.cpp +++ b/Quotient/jobs/basejob.cpp @@ -20,8 +20,7 @@ #include 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( - { { 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,9 +620,15 @@ 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 @@ -643,9 +636,9 @@ 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(*d->backoffStrategy.maxRetries) + : std::numeric_limits().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 index 130a25aa1..c31294b63 100644 --- 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 jobTimeouts; + QVector nextRetryIntervals; + //! How many times a network request should be tried; std::nullopt means keep trying forever + std::optional 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 index edba57f23..21cf40e11 100644 --- 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(query, u"filter"_s, filter); addParam(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(query, u"since"_s, since); setRequestQuery(query); - - setMaxRetries(std::numeric_limits::max()); } SyncJob::SyncJob(const QString& since, const Filter& filter, int timeout, diff --git a/Quotient/jobs/syncjob.h b/Quotient/jobs/syncjob.h index 2e10c82f9..5eb48fc64 100644 --- 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); }