Ensure we remove a child job from before we trigger the next

changelog/unreleased/9725

@@ -0,0 +1,6 @@
+Bugfix: Fix never ending sync
+
+Under certain conditions an upload could enter a state were it would never finish.
+The client would wait for that upload so only a restart of the client or a manual abort of the sync could resolve the issue.
+
+https://github.com/owncloud/client/issues/9725

src/gui/thumbnailjob.cpp

@@ -31,7 +31,7 @@ void ThumbnailJob::start()
     AbstractNetworkJob::start();
 }
 
-bool ThumbnailJob::finished()
+void ThumbnailJob::finished()
 {
     const auto result = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
     QPixmap p;
@@ -42,6 +42,5 @@ bool ThumbnailJob::finished()
         }
     }
     emit jobFinished(result, p);
-    return true;
 }
 }

src/gui/thumbnailjob.h

@@ -44,7 +44,7 @@ public slots:
      */
     void jobFinished(int statusCode, QPixmap reply);
 private slots:
-    bool finished() override;
+    void finished() override;
 };
 }
 

src/libsync/abstractnetworkjob.cpp

@@ -218,12 +218,11 @@ void AbstractNetworkJob::slotFinished()
             qCWarning(lcNetworkJob) << "Don't retry:" << _reply->url();
         }
     }
-
-    bool discard = finished();
-    if (discard) {
-        qCDebug(lcNetworkJob) << "Network job finished" << this;
-        deleteLater();
-    }
+    Q_EMIT aboutToFinishSignal(AbstractNetworkJob::QPrivateSignal());
+    finished();
+    Q_EMIT finishedSignal(AbstractNetworkJob::QPrivateSignal());
+    qCDebug(lcNetworkJob) << "Network job finished" << this;
+    deleteLater();
 }
 
 QByteArray AbstractNetworkJob::responseTimestamp()

src/libsync/abstractnetworkjob.h

@@ -133,6 +133,12 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject
      */
     void networkError(QNetworkReply *reply);
 
+    /**
+     * The job is done
+     */
+    void aboutToFinishSignal(QPrivateSignal);
+    void finishedSignal(QPrivateSignal);
+
 protected:
     /** Initiate a network request, returning a QNetworkReply.
      *
@@ -153,10 +159,8 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject
     virtual void newReplyHook(QNetworkReply *) {}
 
     /** Called at the end of QNetworkReply::finished processing.
-     *
-     * Returning true triggers a deleteLater() of this job.
      */
-    virtual bool finished() = 0;
+    virtual void finished() = 0;
 
     QByteArray _responseTimestamp;
 

src/libsync/bandwidthmanager.cpp

@@ -120,10 +120,12 @@ void BandwidthManager::unregisterUploadDevice(QObject *o)
     }
 }
 
-void BandwidthManager::registerDownloadJob(GETJob *j)
+void BandwidthManager::registerDownloadJob(GETFileJob *j)
 {
     _downloadJobList.push_back(j);
-    QObject::connect(j, &QObject::destroyed, this, &BandwidthManager::unregisterDownloadJob);
+    connect(j, &GETFileJob::aboutToFinishSignal, this, [j, this] {
+        unregisterDownloadJob(j);
+    });
 
     if (usingAbsoluteDownloadLimit()) {
         j->setBandwidthLimited(true);
@@ -137,9 +139,10 @@ void BandwidthManager::registerDownloadJob(GETJob *j)
     }
 }
 
-void BandwidthManager::unregisterDownloadJob(QObject *o)
+void BandwidthManager::unregisterDownloadJob(GETFileJob *j)
 {
-    GETJob *j = reinterpret_cast<GETJob *>(o); // note, we might already be in the ~QObject
+    j->setChoked(false);
+    j->setBandwidthLimited(false);
     _downloadJobList.remove(j);
     if (_relativeLimitCurrentMeasuredJob == j) {
         _relativeLimitCurrentMeasuredJob = nullptr;

src/libsync/bandwidthmanager.h

@@ -24,7 +24,7 @@
 namespace OCC {
 
 class UploadDevice;
-class GETJob;
+class GETFileJob;
 class OwncloudPropagator;
 
 /**
@@ -48,8 +48,8 @@ public slots:
     void registerUploadDevice(UploadDevice *);
     void unregisterUploadDevice(QObject *);
 
-    void registerDownloadJob(GETJob *);
-    void unregisterDownloadJob(QObject *);
+    void registerDownloadJob(GETFileJob *);
+    void unregisterDownloadJob(GETFileJob *);
 
     void absoluteLimitTimerExpired();
     void switchingTimerExpired();
@@ -87,14 +87,14 @@ public slots:
     qint64 _relativeUploadLimitProgressAtMeasuringRestart;
     qint64 _currentUploadLimit;
 
-    std::list<GETJob *> _downloadJobList;
+    std::list<GETFileJob *> _downloadJobList;
     QTimer _relativeDownloadMeasuringTimer;
 
     // for relative bw limiting, we need to wait this amount before measuring again
     QTimer _relativeDownloadDelayTimer;
 
     // the device measured
-    GETJob *_relativeLimitCurrentMeasuredJob;
+    GETFileJob *_relativeLimitCurrentMeasuredJob;
 
     // for measuring how much progress we made at start
     qint64 _relativeDownloadLimitProgressAtMeasuringRestart;

src/libsync/networkjobs.cpp

@@ -95,7 +95,7 @@ void RequestEtagJob::start()
     AbstractNetworkJob::start();
 }
 
-bool RequestEtagJob::finished()
+void RequestEtagJob::finished()
 {
     qCInfo(lcEtagJob) << "Request Etag of" << reply()->request().url() << "FINISHED WITH STATUS"
                       <<  replyStatusString();
@@ -126,7 +126,6 @@ bool RequestEtagJob::finished()
     } else {
         emit finishedWithResult(HttpError{ httpCode, errorString() });
     }
-    return true;
 }
 
 /*********************************************************************************************/
@@ -152,7 +151,7 @@ void MkColJob::start()
     AbstractNetworkJob::start();
 }
 
-bool MkColJob::finished()
+void MkColJob::finished()
 {
     qCInfo(lcMkColJob) << "MKCOL of" << reply()->request().url() << "FINISHED WITH STATUS"
                        << replyStatusString();
@@ -162,7 +161,6 @@ bool MkColJob::finished()
     } else {
         Q_EMIT finishedWithoutError();
     }
-    return true;
 }
 
 /*********************************************************************************************/
@@ -326,7 +324,7 @@ void LsColJob::start()
 // TODO: Instead of doing all in this slot, we should iteratively parse in readyRead(). This
 // would allow us to be more asynchronous in processing while data is coming from the network,
 // not all in one big blob at the end.
-bool LsColJob::finished()
+void LsColJob::finished()
 {
     qCInfo(lcLsColJob) << "LSCOL of" << reply()->request().url() << "FINISHED WITH STATUS"
                        << replyStatusString();
@@ -356,8 +354,6 @@ bool LsColJob::finished()
         // wrong HTTP code or any other network error
         emit finishedWithError(reply());
     }
-
-    return true;
 }
 
 void LsColJob::startImpl(const QNetworkRequest &req)
@@ -449,7 +445,7 @@ QPixmap AvatarJob::makeCircularAvatar(const QPixmap &baseAvatar)
     return avatar;
 }
 
-bool AvatarJob::finished()
+void AvatarJob::finished()
 {
     int http_result_code = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
 
@@ -464,7 +460,6 @@ bool AvatarJob::finished()
         }
     }
     emit avatarPixmap(avImage);
-    return true;
 }
 #endif
 
@@ -481,10 +476,9 @@ void EntityExistsJob::start()
     AbstractNetworkJob::start();
 }
 
-bool EntityExistsJob::finished()
+void EntityExistsJob::finished()
 {
     emit exists(reply());
-    return true;
 }
 
 /*********************************************************************************************/
@@ -510,7 +504,7 @@ void DetermineAuthTypeJob::start()
     AbstractNetworkJob::start();
 }
 
-bool DetermineAuthTypeJob::finished()
+void DetermineAuthTypeJob::finished()
 {
     auto authChallenge = reply()->rawHeader("WWW-Authenticate").toLower();
     auto result = AuthType::Basic;
@@ -521,7 +515,6 @@ bool DetermineAuthTypeJob::finished()
     }
     qCInfo(lcDetermineAuthTypeJob) << "Auth type for" << _account->davUrl() << "is" << result;
     emit this->authType(result);
-    return true;
 }
 
 SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QNetworkRequest &req, QObject *parent)
@@ -588,13 +581,11 @@ void SimpleNetworkJob::addNewReplyHook(std::function<void(QNetworkReply *)> &&ho
     _replyHooks.push_back(hook);
 }
 
-bool SimpleNetworkJob::finished()
+void SimpleNetworkJob::finished()
 {
     if (_device) {
         _device->close();
     }
-    emit finishedSignal();
-    return true;
 }
 
 void SimpleNetworkJob::newReplyHook(QNetworkReply *reply)

src/libsync/networkjobs.h

@@ -53,7 +53,7 @@ class OWNCLOUDSYNC_EXPORT EntityExistsJob : public AbstractNetworkJob
     void exists(QNetworkReply *);
 
 private slots:
-    bool finished() override;
+    void finished() override;
 };
 
 /**
@@ -102,7 +102,7 @@ class OWNCLOUDSYNC_EXPORT LsColJob : public AbstractNetworkJob
     void finishedWithoutError();
 
 private slots:
-    bool finished() override;
+    void finished() override;
 
 protected:
     void startImpl(const QNetworkRequest &req);
@@ -167,7 +167,7 @@ class OWNCLOUDSYNC_EXPORT AvatarJob : public AbstractNetworkJob
     void avatarPixmap(const QPixmap &);
 
 private slots:
-    bool finished() override;
+    void finished() override;
 };
 #endif
 
@@ -190,7 +190,7 @@ class OWNCLOUDSYNC_EXPORT MkColJob : public AbstractNetworkJob
     void finishedWithoutError();
 
 private:
-    bool finished() override;
+    void finished() override;
 };
 
 /**
@@ -208,7 +208,7 @@ class OWNCLOUDSYNC_EXPORT RequestEtagJob : public AbstractNetworkJob
     void finishedWithResult(const HttpResult<QByteArray> &etag);
 
 private slots:
-    bool finished() override;
+    void finished() override;
 };
 
 /**
@@ -232,7 +232,7 @@ class OWNCLOUDSYNC_EXPORT DetermineAuthTypeJob : public AbstractNetworkJob
     void authType(AuthType);
 
 protected Q_SLOTS:
-    bool finished() override;
+    void finished() override;
 };
 
 /**
@@ -259,11 +259,8 @@ class OWNCLOUDSYNC_EXPORT SimpleNetworkJob : public AbstractNetworkJob
 
     void addNewReplyHook(std::function<void(QNetworkReply *)> &&hook);
 
-signals:
-    void finishedSignal();
-
 protected:
-    bool finished() override;
+    void finished() override;
     void newReplyHook(QNetworkReply *) override;
 
     QNetworkRequest _request;

src/libsync/networkjobs/jsonjob.cpp

@@ -28,7 +28,7 @@ JsonJob::~JsonJob()
 {
 }
 
-bool JsonJob::finished()
+void JsonJob::finished()
 {
     qCInfo(lcJsonApiJob) << "JsonJob of" << reply()->request().url() << "FINISHED WITH STATUS"
                          << replyStatusString();
@@ -38,7 +38,7 @@ bool JsonJob::finished()
     } else {
         parse(reply()->readAll());
     }
-    return SimpleNetworkJob::finished();
+    SimpleNetworkJob::finished();
 }
 
 void JsonJob::parse(const QByteArray &data)

src/libsync/networkjobs/jsonjob.h

@@ -33,7 +33,7 @@ class OWNCLOUDSYNC_EXPORT JsonJob : public SimpleNetworkJob
     const QJsonParseError &parseError() const;
 
 protected:
-    bool finished() override;
+    void finished() override;
 
     virtual void parse(const QByteArray &data);