From a63d970e5ef39b6de3f55a75a4cabb8b4bbd18dc Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 20 Jan 2017 13:59:13 +0100 Subject: [PATCH] ChunkingNG: remove stale chunks when cleaning the uploadInfo table Stale chunks might be there because a file was removed or would just not be uploaded, for any reason. We just start the DeleteJob but we don't care if it success or not. Relates to https://github.com/owncloud/core/issues/26981 One of the test is testing the case where the file is modified on the server during the upload. So this test the precondition failed error. The FakeGetReply logic was modified because resizing a 150MB big QByteArray by increment of 16k just did not scale when downloading a big file. --- src/libsync/syncengine.cpp | 12 +++++- src/libsync/syncjournaldb.cpp | 13 ++++--- src/libsync/syncjournaldb.h | 3 +- test/syncenginetestutils.h | 30 ++++++++++---- test/testchunkingng.cpp | 73 ++++++++++++++++++++++++++++++++++- 5 files changed, 115 insertions(+), 16 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index a4e2fceab..52cc9a5ff 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -23,6 +23,7 @@ #include "syncfilestatus.h" #include "csync_private.h" #include "filesystem.h" +#include "propagateremotedelete.h" #ifdef Q_OS_WIN #include @@ -301,7 +302,16 @@ void SyncEngine::deleteStaleUploadInfos() } // Delete from journal. - _journal->deleteStaleUploadInfos(upload_file_paths); + auto ids = _journal->deleteStaleUploadInfos(upload_file_paths); + + // Delete the stales chunk on the server. + if (account()->capabilities().chunkingNg()) { + foreach (uint transferId, ids) { + QUrl url = Utility::concatUrlPath(account()->url(), QLatin1String("remote.php/dav/uploads/") + + account()->davUser() + QLatin1Char('/') + QString::number(transferId)); + (new DeleteJob(account(), url, this))->start(); + } + } } void SyncEngine::deleteStaleErrorBlacklistEntries() diff --git a/src/libsync/syncjournaldb.cpp b/src/libsync/syncjournaldb.cpp index 74fd86b22..ae0cb8e26 100644 --- a/src/libsync/syncjournaldb.cpp +++ b/src/libsync/syncjournaldb.cpp @@ -1370,21 +1370,22 @@ void SyncJournalDb::setUploadInfo(const QString& file, const SyncJournalDb::Uplo } } -bool SyncJournalDb::deleteStaleUploadInfos(const QSet &keep) +QVector SyncJournalDb::deleteStaleUploadInfos(const QSet &keep) { QMutexLocker locker(&_mutex); + QVector ids; if (!checkConnect()) { - return false; + return ids; } SqlQuery query(_db); - query.prepare("SELECT path FROM uploadinfo"); + query.prepare("SELECT path,transferid FROM uploadinfo"); if (!query.exec()) { QString err = query.error(); qDebug() << "Error creating prepared statement: " << query.lastQuery() << ", Error:" << err; - return false; + return ids; } QStringList superfluousPaths; @@ -1393,10 +1394,12 @@ bool SyncJournalDb::deleteStaleUploadInfos(const QSet &keep) const QString file = query.stringValue(0); if (!keep.contains(file)) { superfluousPaths.append(file); + ids.append(query.intValue(1)); } } - return deleteBatch(*_deleteUploadInfoQuery, superfluousPaths, "uploadinfo"); + deleteBatch(*_deleteUploadInfoQuery, superfluousPaths, "uploadinfo"); + return ids; } SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry( const QString& file ) diff --git a/src/libsync/syncjournaldb.h b/src/libsync/syncjournaldb.h index 1f2ba6d8b..fe00051a3 100644 --- a/src/libsync/syncjournaldb.h +++ b/src/libsync/syncjournaldb.h @@ -105,7 +105,8 @@ public: UploadInfo getUploadInfo(const QString &file); void setUploadInfo(const QString &file, const UploadInfo &i); - bool deleteStaleUploadInfos(const QSet& keep); + // Return the list of transfer ids that were removed. + QVector deleteStaleUploadInfos(const QSet& keep); SyncJournalErrorBlacklistRecord errorBlacklistEntry( const QString& ); bool deleteStaleErrorBlacklistEntries(const QSet& keep); diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index b2e52444b..f33096352 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -524,7 +524,8 @@ class FakeGetReply : public QNetworkReply Q_OBJECT public: const FileInfo *fileInfo; - QByteArray payload; + char payload; + int size; FakeGetReply(FileInfo &remoteRootFileInfo, QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent) : QNetworkReply{parent} { @@ -540,8 +541,9 @@ public: } Q_INVOKABLE void respond() { - payload.fill(fileInfo->contentChar, fileInfo->size); - setHeader(QNetworkRequest::ContentLengthHeader, payload.size()); + payload = fileInfo->contentChar; + size = fileInfo->size; + setHeader(QNetworkRequest::ContentLengthHeader, size); setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 200); setRawHeader("OC-ETag", fileInfo->etag.toLatin1()); setRawHeader("ETag", fileInfo->etag.toLatin1()); @@ -553,12 +555,12 @@ public: } void abort() override { } - qint64 bytesAvailable() const override { return payload.size() + QIODevice::bytesAvailable(); } + qint64 bytesAvailable() const override { return size + QIODevice::bytesAvailable(); } qint64 readData(char *data, qint64 maxlen) override { - qint64 len = std::min(qint64{payload.size()}, maxlen); - strncpy(data, payload.constData(), len); - payload.remove(0, len); + qint64 len = std::min(qint64{size}, maxlen); + std::fill_n(data, len, payload); + size -= len; return len; } }; @@ -607,7 +609,12 @@ public: Q_ASSERT(!fileName.isEmpty()); if ((fileInfo = remoteRootFileInfo.find(fileName))) { - QCOMPARE(request.rawHeader("If"), QByteArray("<" + request.rawHeader("Destination") + "> ([\"" + fileInfo->etag.toLatin1() + "\"])")); + QVERIFY(request.hasRawHeader("If")); // The client should put this header + if (request.rawHeader("If") != QByteArray("<" + request.rawHeader("Destination") + + "> ([\"" + fileInfo->etag.toLatin1() + "\"])")) { + QMetaObject::invokeMethod(this, "respondPreconditionFailed", Qt::QueuedConnection); + return; + } fileInfo->size = size; fileInfo->contentChar = payload; } else { @@ -632,6 +639,13 @@ public: emit finished(); } + Q_INVOKABLE void respondPreconditionFailed() { + setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 412); + setError(InternalServerError, "Precondition Failed"); + emit metaDataChanged(); + emit finished(); + } + void abort() override { } qint64 readData(char *, qint64) override { return 0; } }; diff --git a/test/testchunkingng.cpp b/test/testchunkingng.cpp index b038737d6..ce7bbe278 100644 --- a/test/testchunkingng.cpp +++ b/test/testchunkingng.cpp @@ -86,7 +86,7 @@ private slots: } // We modify the file locally after it has been partially uploaded - void testRemoveStale() { + void testRemoveStale1() { FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } }); @@ -107,6 +107,77 @@ private slots: QCOMPARE(fakeFolder.uploadState().children.count(), 1); QVERIFY(fakeFolder.uploadState().children.first().name != chunkingId); } + + // We remove the file locally after it has been partially uploaded + void testRemoveStale2() { + + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } }); + const int size = 300 * 1000 * 1000; // 300 MB + partialUpload(fakeFolder, "A/a0", size); + QCOMPARE(fakeFolder.uploadState().children.count(), 1); + + fakeFolder.localModifier().remove("A/a0"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.uploadState().children.count(), 0); + } + + + void testCreateConflictWhileSyncing() { + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } }); + const int size = 150 * 1000 * 1000; // 150 MB + + // Put a file on the server and download it. + fakeFolder.remoteModifier().insert("A/a0", size); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + // Modify the file localy and start the upload + fakeFolder.localModifier().setContents("A/a0", 'B'); + fakeFolder.localModifier().appendByte("A/a0"); + + // But in the middle of the sync, modify the file on the server + QMetaObject::Connection con = QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::transmissionProgress, + [&](const ProgressInfo &progress) { + if (progress.completedSize() > (progress.totalSize() / 2 )) { + fakeFolder.remoteModifier().setContents("A/a0", 'C'); + QObject::disconnect(con); + } + }); + + QVERIFY(!fakeFolder.syncOnce()); + // There was a precondition failed error, this means wen need to sync again + QCOMPARE(fakeFolder.syncEngine().isAnotherSyncNeeded(), ImmediateFollowUp); + + QCOMPARE(fakeFolder.uploadState().children.count(), 1); // We did not clean the chunks at this point + + // Now we will download the server file and create a conflict + QVERIFY(fakeFolder.syncOnce()); + auto localState = fakeFolder.currentLocalState(); + + // A0 is the one from the server + QCOMPARE(localState.find("A/a0")->size, size); + QCOMPARE(localState.find("A/a0")->contentChar, 'C'); + + // There is a conflict file with our version + auto &stateAChildren = localState.find("A")->children; + auto it = std::find_if(stateAChildren.cbegin(), stateAChildren.cend(), [&](const FileInfo &fi) { + return fi.name.startsWith("a0_conflict"); + }); + QVERIFY(it != stateAChildren.cend()); + QCOMPARE(it->contentChar, 'B'); + QCOMPARE(it->size, size+1); + + // Remove the conflict file so the comparison works! + fakeFolder.localModifier().remove("A/" + it->name); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + QCOMPARE(fakeFolder.uploadState().children.count(), 0); // The last sync cleaned the chunks + } + }; QTEST_GUILESS_MAIN(TestChunkingNG)