From: Christian Kamm Date: Fri, 30 Aug 2019 10:05:50 +0000 (+0200) Subject: Propagation: Fix delete-before-rename bug #7441 X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~189 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=c9d103762284958345f40bac59be2b82c9de880b;p=nextcloud-desktop.git Propagation: Fix delete-before-rename bug #7441 By introducing a PropagateRootDirectory job that explicitly separates the directory deletion jobs from all the other jobs. Note that this means that if there are errors in subJobs the dirDeletionJobs won't get executed. --- diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 856b2e25f..3fbbd8f03 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -373,7 +373,7 @@ void OwncloudPropagator::start(const SyncFileItemVector &items) * In order to do that we loop over the items. (which are sorted by destination) * When we enter a directory, we can create the directory job and push it on the stack. */ - _rootJob.reset(new PropagateDirectory(this)); + _rootJob.reset(new PropagateRootDirectory(this)); QStack> directories; directories.push(qMakePair(QString(), _rootJob.data())); QVector directoriesToRemove; @@ -482,7 +482,7 @@ void OwncloudPropagator::start(const SyncFileItemVector &items) } foreach (PropagatorJob *it, directoriesToRemove) { - _rootJob->appendJob(it); + _rootJob->_dirDeletionJobs.appendJob(it); } connect(_rootJob.data(), &PropagatorJob::finished, this, &OwncloudPropagator::emitFinished); @@ -993,6 +993,91 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) emit finished(status); } +PropagateRootDirectory::PropagateRootDirectory(OwncloudPropagator *propagator) + : PropagateDirectory(propagator, SyncFileItemPtr(new SyncFileItem)) + , _dirDeletionJobs(propagator) +{ + connect(&_dirDeletionJobs, &PropagatorJob::finished, this, &PropagateRootDirectory::slotDirDeletionJobsFinished); +} + +PropagatorJob::JobParallelism PropagateRootDirectory::parallelism() +{ + // the root directory parallelism isn't important + return WaitForFinished; +} + +void PropagateRootDirectory::abort(PropagatorJob::AbortType abortType) +{ + if (_firstJob) + // Force first job to abort synchronously + // even if caller allows async abort (asyncAbort) + _firstJob->abort(AbortType::Synchronous); + + if (abortType == AbortType::Asynchronous) { + struct AbortsFinished { + bool subJobsFinished = false; + bool dirDeletionFinished = false; + }; + auto abortStatus = QSharedPointer(new AbortsFinished); + + connect(&_subJobs, &PropagatorCompositeJob::abortFinished, this, [this, abortStatus]() { + abortStatus->subJobsFinished = true; + if (abortStatus->subJobsFinished && abortStatus->dirDeletionFinished) + emit abortFinished(); + }); + connect(&_dirDeletionJobs, &PropagatorCompositeJob::abortFinished, this, [this, abortStatus]() { + abortStatus->dirDeletionFinished = true; + if (abortStatus->subJobsFinished && abortStatus->dirDeletionFinished) + emit abortFinished(); + }); + } + _subJobs.abort(abortType); + _dirDeletionJobs.abort(abortType); +} + +qint64 PropagateRootDirectory::committedDiskSpace() const +{ + return _subJobs.committedDiskSpace() + _dirDeletionJobs.committedDiskSpace(); +} + +bool PropagateRootDirectory::scheduleSelfOrChild() +{ + if (_state == Finished) + return false; + + if (PropagateDirectory::scheduleSelfOrChild()) + return true; + + // Important: Finish _subJobs before scheduling any deletes. + if (_subJobs._state != Finished) + return false; + + return _dirDeletionJobs.scheduleSelfOrChild(); +} + +void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) +{ + if (status != SyncFileItem::Success + && status != SyncFileItem::Restoration + && status != SyncFileItem::Conflict) { + if (_state != Finished) { + // Synchronously abort + abort(AbortType::Synchronous); + _state = Finished; + emit finished(status); + } + return; + } + + propagator()->scheduleNextJob(); +} + +void PropagateRootDirectory::slotDirDeletionJobsFinished(SyncFileItem::Status status) +{ + _state = Finished; + emit finished(status); +} + // ================================================================================ CleanupPollsJob::~CleanupPollsJob() = default; diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index c5431f051..8c874a019 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -289,7 +289,7 @@ public: PropagatorCompositeJob _subJobs; - explicit PropagateDirectory(OwncloudPropagator *propagator, const SyncFileItemPtr &item = SyncFileItemPtr(new SyncFileItem)); + explicit PropagateDirectory(OwncloudPropagator *propagator, const SyncFileItemPtr &item); void appendJob(PropagatorJob *job) { @@ -330,10 +330,35 @@ public: private slots: void slotFirstJobFinished(SyncFileItem::Status status); - void slotSubJobsFinished(SyncFileItem::Status status); + virtual void slotSubJobsFinished(SyncFileItem::Status status); }; +/** + * @brief Propagate the root directory, and all its sub entries. + * @ingroup libsync + * + * Primary difference to PropagateDirectory is that it keeps track of directory + * deletions that must happen at the very end. + */ +class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory +{ + Q_OBJECT +public: + PropagatorCompositeJob _dirDeletionJobs; + + explicit PropagateRootDirectory(OwncloudPropagator *propagator); + + bool scheduleSelfOrChild() override; + JobParallelism parallelism() override; + void abort(PropagatorJob::AbortType abortType) override; + + qint64 committedDiskSpace() const override; + +private slots: + void slotSubJobsFinished(SyncFileItem::Status status) override; + void slotDirDeletionJobsFinished(SyncFileItem::Status status); +}; /** * @brief Dummy job that just mark it as completed and ignored @@ -565,7 +590,7 @@ signals: private: AccountPtr _account; - QScopedPointer _rootJob; + QScopedPointer _rootJob; SyncOptions _syncOptions; bool _jobScheduled = false; }; diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index 8f349adc8..276e34435 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -1190,20 +1190,26 @@ namespace OCC { inline void addFiles(QStringList &dest, const FileInfo &fi) { if (fi.isDir) { - dest += QString("%1 - dir").arg(fi.name); + dest += QString("%1 - dir").arg(fi.path()); foreach (const FileInfo &fi, fi.children) addFiles(dest, fi); } else { - dest += QString("%1 - %2 %3-bytes").arg(fi.name).arg(fi.size).arg(fi.contentChar); + dest += QString("%1 - %2 %3-bytes").arg(fi.path()).arg(fi.size).arg(fi.contentChar); } } -inline char *toString(const FileInfo &fi) +inline QString toStringNoElide(const FileInfo &fi) { QStringList files; foreach (const FileInfo &fi, fi.children) addFiles(files, fi); - return QTest::toString(QString("FileInfo with %1 files(%2)").arg(files.size()).arg(files.join(", "))); + files.sort(); + return QString("FileInfo with %1 files(\n\t%2\n)").arg(files.size()).arg(files.join("\n\t")); +} + +inline char *toString(const FileInfo &fi) +{ + return QTest::toString(toStringNoElide(fi)); } inline void addFilesDbData(QStringList &dest, const FileInfo &fi) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 7cab63359..ad02ca0a9 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -243,10 +243,13 @@ private slots: //2. // old removed QVERIFY(!currentLocalState.find("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_")); + // but still on the server: the rename causing an error meant the deletes didn't execute + QVERIFY(fakeFolder.currentRemoteState().find("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_")); // new still there QVERIFY(currentLocalState.find("readonlyDirectory_PERM_M_/moved_PERM_CK_/subsubdir_PERM_CKDNV_/normalFile_PERM_WVND_.data" )); //but not on server fakeFolder.localModifier().remove("readonlyDirectory_PERM_M_/moved_PERM_CK_"); + fakeFolder.remoteModifier().remove("normalDirectory_PERM_CKDNV_/subdir_PERM_CKDNV_"); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 390dd0104..43206d96f 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -816,6 +816,23 @@ private slots: QCOMPARE(counter.nPUT, 0); QCOMPARE(counter.nMOVE, 2); } + + // Test that deletes don't run before renames + void testRenameParallelism() + { + FakeFolder fakeFolder{ FileInfo{} }; + fakeFolder.remoteModifier().mkdir("A"); + fakeFolder.remoteModifier().insert("A/file"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.localModifier().mkdir("B"); + fakeFolder.localModifier().rename("A/file", "B/file"); + fakeFolder.localModifier().remove("A"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } }; QTEST_GUILESS_MAIN(TestSyncMove)