fix(network): enable again timeout for network requests

src/libsync/abstractnetworkjob.cpp

@@ -1,10 +1,10 @@
 /*
  * SPDX-FileCopyrightText: 2018 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2015 ownCloud GmbH
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
 #include "common/asserts.h"
 #include "networkjobs.h"
 #include "account.h"
 #include "owncloudpropagator.h"
@@ -38,7 +38,7 @@
 
 // If not set, it is overwritten by the Application constructor with the value from the config
 int AbstractNetworkJob::httpTimeout = qEnvironmentVariableIntValue("OWNCLOUD_TIMEOUT");
-bool AbstractNetworkJob::enableTimeout = false;
+bool AbstractNetworkJob::enableTimeout = true;
 
 AbstractNetworkJob::AbstractNetworkJob(const AccountPtr &account, const QString &path, QObject *parent)
     : QObject(parent)
@@ -48,19 +48,6 @@
 {
     // Since we hold a QSharedPointer to the account, this makes no sense. (issue #6893)
     ASSERT(account != parent);
-
-    _timer.setSingleShot(true);
-    _timer.setInterval((httpTimeout ? httpTimeout : 300) * 1000); // default to 5 minutes.
-    connect(&_timer, &QTimer::timeout, this, &AbstractNetworkJob::slotTimeout);
-
-    connect(this, &AbstractNetworkJob::networkActivity, this, &AbstractNetworkJob::resetTimeout);
-
-    // Network activity on the propagator jobs (GET/PUT) keeps all requests alive.
-    // This is a workaround for OC instances which only support one
-    // parallel up and download
-    if (_account) {
-        connect(_account.data(), &Account::propagatorNetworkActivity, this, &AbstractNetworkJob::resetTimeout);
-    }
 }
 
 void AbstractNetworkJob::setReply(QNetworkReply *reply)
@@ -75,14 +62,7 @@
 
 void AbstractNetworkJob::setTimeout(qint64 msec)
 {
-    _timer.start(msec);
-}
-
-void AbstractNetworkJob::resetTimeout()
-{
-    qint64 interval = _timer.interval();
-    _timer.stop();
-    _timer.start(interval);
+    _timeoutDuration = msec;
 }
 
 void AbstractNetworkJob::setIgnoreCredentialFailure(bool ignore)
@@ -112,17 +92,15 @@
     connect(reply, &QNetworkReply::redirected, this, [reply, this] (const QUrl &url) { emit redirected(reply, url, 0);});
 }
 
-QNetworkReply *AbstractNetworkJob::addTimer(QNetworkReply *reply)
-{
-    reply->setProperty("timer", QVariant::fromValue(&_timer));
-    return reply;
-}
-
 QNetworkReply *AbstractNetworkJob::sendRequest(const QByteArray &verb,
                                                const QUrl &url,
                                                QNetworkRequest req,
                                                QIODevice *requestBody)
 {
+    if (AbstractNetworkJob::enableTimeout) {
+        req.setTransferTimeout(_timeoutDuration);
+    }
+
     auto reply = _account->sendRawRequest(verb, url, req, requestBody);
     _requestBody = requestBody;
     if (_requestBody) {
@@ -137,6 +115,10 @@
                                                QNetworkRequest req,
                                                const QByteArray &requestBody)
 {
+    if (AbstractNetworkJob::enableTimeout) {
+        req.setTransferTimeout(_timeoutDuration);
+    }
+
     auto reply = _account->sendRawRequest(verb, url, req, requestBody);
     _requestBody = nullptr;
     adoptRequest(reply);
@@ -148,6 +130,10 @@
                                                QNetworkRequest req,
                                                QHttpMultiPart *requestBody)
 {
+    if (AbstractNetworkJob::enableTimeout) {
+        req.setTransferTimeout(_timeoutDuration);
+    }
+
     auto reply = _account->sendRawRequest(verb, url, req, requestBody);
     _requestBody = nullptr;
     adoptRequest(reply);
@@ -156,7 +142,6 @@
 
 void AbstractNetworkJob::adoptRequest(QNetworkReply *reply)
 {
-    addTimer(reply);
     setReply(reply);
     setupConnections(reply);
     newReplyHook(reply);
@@ -172,9 +157,21 @@
     return Utility::concatUrlPath(_account->davUrl(), relativePath);
 }
 
+void AbstractNetworkJob::onTimedOut()
+{
+    if (reply()) {
+        reply()->abort();
+    } else {
+        deleteLater();
+    }
+}
+
 void AbstractNetworkJob::slotFinished()
 {
-    _timer.stop();
+    if (_reply->error() == QNetworkReply::TimeoutError) {
+        qCWarning(lcNetworkJob) << "TimeoutError"  << errorString();
+        onTimedOut();
+    }
 
     if (_reply->error() == QNetworkReply::SslHandshakeFailedError) {
         qCWarning(lcNetworkJob) << "SslHandshakeFailedError: " << errorString() << " : can be caused by a webserver wanting SSL client certificates";
@@ -195,7 +192,6 @@
             qCInfo(lcNetworkJob) << "HTTP2 resending" << _reply->request().url();
             _http2ResendCount++;
 
-            resetTimeout();
             if (_requestBody) {
                 if(!_requestBody->isOpen())
                    _requestBody->open(QIODevice::ReadOnly);
@@ -265,7 +261,6 @@
 
                 // Create the redirected request and send it
                 qCInfo(lcNetworkJob) << "Redirecting" << verb << requestedUrl << redirectUrl;
-                resetTimeout();
                 if (_requestBody) {
                     if(!_requestBody->isOpen()) {
                         // Avoid the QIODevice::seek (QBuffer): The device is not open warning message
@@ -321,10 +316,6 @@
 
 QString AbstractNetworkJob::errorString() const
 {
-    if (_timedout) {
-        return tr("The server took too long to respond. Check your connection and try syncing again. If it still doesn’t work, reach out to your server administrator.");
-    }
-
     if (!reply()) {
         return tr("An unexpected error occurred. Please try syncing again or contact your server administrator if the issue continues.");
     }
@@ -373,36 +364,13 @@
 
 void AbstractNetworkJob::start()
 {
-    _timer.start();
-
     const QUrl url = account()->url();
     const QString displayUrl = QStringLiteral("%1://%2%3").arg(url.scheme()).arg(url.host()).arg(url.path());
 
     QString parentMetaObjectName = parent() ? parent()->metaObject()->className() : "";
     qCInfo(lcNetworkJob) << metaObject()->className() << "created for" << displayUrl << "+" << path() << parentMetaObjectName;
 }
 
-void AbstractNetworkJob::slotTimeout()
-{
-    // TODO: workaround, find cause of https://github.com/nextcloud/desktop/issues/7184
-    if (!AbstractNetworkJob::enableTimeout) {
-        return;
-    }
-
-    _timedout = true;
-    qCWarning(lcNetworkJob) << "Network job timeout" << (reply() ? reply()->request().url() : path());
-    onTimedOut();
-}
-
-void AbstractNetworkJob::onTimedOut()
-{
-    if (reply()) {
-        reply()->abort();
-    } else {
-        deleteLater();
-    }
-}
-
 QString AbstractNetworkJob::replyStatusString() {
     Q_ASSERT(reply());
     if (reply()->error() == QNetworkReply::NoError) {
@@ -579,7 +547,6 @@
     QUrl requestedUrl = req.url();
     QByteArray verb = HttpLogger::requestVerb(*_reply);
     qCInfo(lcNetworkJob) << "Restarting" << verb << requestedUrl;
-    resetTimeout();
     if (_requestBody) {
         _requestBody->seek(0);
     }

src/libsync/abstractnetworkjob.h

@@ -6,7 +6,7 @@
 
 #pragma once
 
 #include "owncloudlib.h"
 
 #include "accountfwd.h"
 #include "common/asserts.h"
@@ -70,8 +70,7 @@
     /* Content of the X-Request-ID header. (Only set after the request is sent) */
     QByteArray requestId();
 
-    [[nodiscard]] qint64 timeoutMsec() const { return _timer.interval(); }
-    [[nodiscard]] bool timedOut() const { return _timedout; }
+    [[nodiscard]] qint64 timeoutMsec() const { return _timeoutDuration; }
 
     /** Returns an error message, if any. */
     [[nodiscard]] virtual QString errorString() const;
@@ -112,7 +111,6 @@
 
 public slots:
     void setTimeout(qint64 msec);
-    void resetTimeout();
 signals:
     /** Emitted on network error.
      *
@@ -191,7 +189,6 @@
     virtual void onTimedOut();
 
     QByteArray _responseTimestamp;
-    bool _timedout = false; // set to true when the timeout slot is received
 
     // Automatically follows redirects. Note that this only works for
     // GET requests that don't set up any HTTP body or other flags.
@@ -201,19 +198,17 @@
 
 private slots:
     void slotFinished();
-    void slotTimeout();
 
 protected:
     AccountPtr _account;
 
 private:
-    QNetworkReply *addTimer(QNetworkReply *reply);
     bool _ignoreCredentialFailure = false;
     QPointer<QNetworkReply> _reply; // (QPointer because the NetworkManager may be destroyed before the jobs at exit)
     QString _path;
-    QTimer _timer;
     int _redirectCount = 0;
     int _http2ResendCount = 0;
+    qint64 _timeoutDuration = 300000;
 
     // Set by the xyzRequest() functions and needed to be able to redirect
     // requests, should it be required.

src/libsync/propagateupload.cpp

@@ -102,7 +102,6 @@ void PollJob::start()
     QUrl finalUrl = QUrl::fromUserInput(accountUrl.scheme() + QLatin1String("://") + accountUrl.authority()
         + (path().startsWith('/') ? QLatin1String("") : QLatin1String("/")) + path());
     sendRequest("GET", finalUrl);
-    connect(reply(), &QNetworkReply::downloadProgress, this, &AbstractNetworkJob::resetTimeout, Qt::UniqueConnection);
     AbstractNetworkJob::start();
 }