From f6c77fad17060bf25f4e6591360340670ba2dd1c Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 20 Jan 2017 12:06:20 +0100 Subject: [PATCH 1/3] ChunkingNG: delete stale chunks if the file was changed locally Relates to https://github.com/owncloud/core/issues/26981 We do not track the success or error of the DeleteJob because it does not matter. If it fails, it might be because the chunks were already removed. If not, the chunks will be stale, but the server must anyway do a few cleanup from time to time because we do not always remove the chunks --- src/libsync/propagateuploadng.cpp | 6 +++ test/syncenginetestutils.h | 3 +- test/testchunkingng.cpp | 81 ++++++++++++++++++++++--------- 3 files changed, 66 insertions(+), 24 deletions(-) diff --git a/src/libsync/propagateuploadng.cpp b/src/libsync/propagateuploadng.cpp index 09891b65b85..b4191a2166b 100644 --- a/src/libsync/propagateuploadng.cpp +++ b/src/libsync/propagateuploadng.cpp @@ -97,6 +97,12 @@ void PropagateUploadFileNG::doStartUpload() this, SLOT(slotPropfindIterate(QString,QMap))); job->start(); return; + } else if (progressInfo._valid) { + // The upload info is stale. remove the stale chunks on the server + _transferId = progressInfo._transferid; + // Fire and forget. Any error will be ignored. + (new DeleteJob(propagator()->account(), chunkUrl(), this))->start(); + // startNewUpload will reset the _transferId and the UploadInfo in the db. } startNewUpload(); diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index 665dc201691..b2e52444bc8 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -586,7 +586,7 @@ class FakeChunkMoveReply : public QNetworkReply Q_ASSERT(sourceFolder->isDir); int count = 0; int size = 0; - char payload = '*'; + char payload = '\0'; do { if (!sourceFolder->children.contains(QString::number(count))) @@ -595,6 +595,7 @@ class FakeChunkMoveReply : public QNetworkReply Q_ASSERT(!x.isDir); Q_ASSERT(x.size > 0); // There should not be empty chunks size += x.size; + Q_ASSERT(!payload || payload == x.contentChar); payload = x.contentChar; ++count; } while(true); diff --git a/test/testchunkingng.cpp b/test/testchunkingng.cpp index 21225c210c7..b038737d621 100644 --- a/test/testchunkingng.cpp +++ b/test/testchunkingng.cpp @@ -11,6 +11,36 @@ using namespace OCC; +/* Upload a 1/3 of a file of given size. + * fakeFolder needs to be synchronized */ +static void partialUpload(FakeFolder &fakeFolder, const QString &name, int size) +{ + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(fakeFolder.uploadState().children.count(), 0); // The state should be clean + + fakeFolder.localModifier().insert(name, size); + // Abort when the upload is at 1/3 + int sizeWhenAbort = -1; + auto con = QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::transmissionProgress, + [&](const ProgressInfo &progress) { + if (progress.completedSize() > (progress.totalSize() /3 )) { + sizeWhenAbort = progress.completedSize(); + fakeFolder.syncEngine().abort(); + } + }); + + QVERIFY(!fakeFolder.syncOnce()); // there should have been an error + QObject::disconnect(con); + QVERIFY(sizeWhenAbort > 0); + QVERIFY(sizeWhenAbort < size); + + QCOMPARE(fakeFolder.uploadState().children.count(), 1); // the transfer was done with chunking + auto upStateChildren = fakeFolder.uploadState().children.first().children; + QCOMPARE(sizeWhenAbort, std::accumulate(upStateChildren.cbegin(), upStateChildren.cend(), 0, + [](int s, const FileInfo &i) { return s + i.size; })); +} + + class TestChunkingNG : public QObject { Q_OBJECT @@ -40,37 +70,42 @@ private slots: FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } }); const int size = 300 * 1000 * 1000; // 300 MB - fakeFolder.localModifier().insert("A/a0", size); - - // Abort when the upload is at 1/3 - int sizeWhenAbort = -1; - auto con = QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::transmissionProgress, - [&](const ProgressInfo &progress) { - if (progress.completedSize() > (progress.totalSize() /3 )) { - sizeWhenAbort = progress.completedSize(); - fakeFolder.syncEngine().abort(); - } - }); - - QVERIFY(!fakeFolder.syncOnce()); // there should have been an error - QObject::disconnect(con); - QVERIFY(sizeWhenAbort > 0); - QVERIFY(sizeWhenAbort < size); - QCOMPARE(fakeFolder.uploadState().children.count(), 1); // the transfer was done with chunking - auto upStateChildren = fakeFolder.uploadState().children.first().children; - QCOMPARE(sizeWhenAbort, std::accumulate(upStateChildren.cbegin(), upStateChildren.cend(), 0, - [](int s, const FileInfo &i) { return s + i.size; })); - + partialUpload(fakeFolder, "A/a0", size); + QCOMPARE(fakeFolder.uploadState().children.count(), 1); + auto chunkingId = fakeFolder.uploadState().children.first().name; // Add a fake file to make sure it gets deleted fakeFolder.uploadState().children.first().insert("10000", size); QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size); + // The same chunk id was re-used + QCOMPARE(fakeFolder.uploadState().children.count(), 1); + QCOMPARE(fakeFolder.uploadState().children.first().name, chunkingId); + } + // We modify the file locally after it has been partially uploaded + void testRemoveStale() { + + 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); + auto chunkingId = fakeFolder.uploadState().children.first().name; + + + fakeFolder.localModifier().setContents("A/a0", 'B'); + fakeFolder.localModifier().appendByte("A/a0"); + + QVERIFY(fakeFolder.syncOnce()); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); - QCOMPARE(fakeFolder.uploadState().children.count(), 1); // The same chunk id was re-used - QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size); + QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size + 1); + // A different chunk id was used, and the previous one is removed + QCOMPARE(fakeFolder.uploadState().children.count(), 1); + QVERIFY(fakeFolder.uploadState().children.first().name != chunkingId); } }; From a63d970e5ef39b6de3f55a75a4cabb8b4bbd18dc Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 20 Jan 2017 13:59:13 +0100 Subject: [PATCH 2/3] 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 a4e2fceabcd..52cc9a5fff4 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 74fd86b2286..ae0cb8e2673 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 1f2ba6d8bec..fe00051a33a 100644 --- a/src/libsync/syncjournaldb.h +++ b/src/libsync/syncjournaldb.h @@ -105,7 +105,8 @@ class OWNCLOUDSYNC_EXPORT SyncJournalDb : public QObject 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 b2e52444bc8..f3309635247 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 @@ class FakeGetReply : public QNetworkReply } 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 @@ class FakeGetReply : public QNetworkReply } 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 @@ class FakeChunkMoveReply : public QNetworkReply 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 @@ class FakeChunkMoveReply : public QNetworkReply 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 b038737d621..ce7bbe278f7 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) From 8a70d22af778c84dc3aece2a1e698b9da3ef5fea Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 20 Jan 2017 14:38:21 +0100 Subject: [PATCH 3/3] ChunkingNG: Add some tests - Test that we recover correctly if the chunks were removed on the server. - Test changing the file localy while uploading. --- test/syncenginetestutils.h | 12 +++++++- test/testchunkingng.cpp | 59 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index f3309635247..a4a12884f03 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -315,7 +315,10 @@ class FakePropfindReply : public QNetworkReply QString fileName = getFilePathFromUrl(request.url()); Q_ASSERT(!fileName.isNull()); // for root, it should be empty const FileInfo *fileInfo = remoteRootFileInfo.find(fileName); - Q_ASSERT(fileInfo); + if (!fileInfo) { + QMetaObject::invokeMethod(this, "respond404", Qt::QueuedConnection); + return; + } QString prefix = request.url().path().left(request.url().path().size() - fileName.size()); // Don't care about the request and just return a full propfind @@ -375,6 +378,13 @@ class FakePropfindReply : public QNetworkReply emit finished(); } + Q_INVOKABLE void respond404() { + setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 404); + setError(InternalServerError, "Not Found"); + emit metaDataChanged(); + emit finished(); + } + void abort() override { } qint64 bytesAvailable() const override { return payload.size() + QIODevice::bytesAvailable(); } diff --git a/test/testchunkingng.cpp b/test/testchunkingng.cpp index ce7bbe278f7..ed30f8954bf 100644 --- a/test/testchunkingng.cpp +++ b/test/testchunkingng.cpp @@ -178,6 +178,65 @@ private slots: QCOMPARE(fakeFolder.uploadState().children.count(), 0); // The last sync cleaned the chunks } + void testModifyLocalFileWhileUploading() { + + FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()}; + fakeFolder.syncEngine().account()->setCapabilities({ { "dav", QVariantMap{ {"chunking", "1.0"} } } }); + const int size = 150 * 1000 * 1000; // 150 MB + + fakeFolder.localModifier().insert("A/a0", size); + + // middle of the sync, modify the file + QMetaObject::Connection con = QObject::connect(&fakeFolder.syncEngine(), &SyncEngine::transmissionProgress, + [&](const ProgressInfo &progress) { + if (progress.completedSize() > (progress.totalSize() / 2 )) { + fakeFolder.localModifier().setContents("A/a0", 'B'); + fakeFolder.localModifier().appendByte("A/a0"); + QObject::disconnect(con); + } + }); + + QVERIFY(!fakeFolder.syncOnce()); + + // There should be a followup sync + QCOMPARE(fakeFolder.syncEngine().isAnotherSyncNeeded(), ImmediateFollowUp); + + QCOMPARE(fakeFolder.uploadState().children.count(), 1); // We did not clean the chunks at this point + auto chunkingId = fakeFolder.uploadState().children.first().name; + + // Now we make a new sync which should upload the file for good. + QVERIFY(fakeFolder.syncOnce()); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size+1); + + // A different chunk id was used, and the previous one is removed + QCOMPARE(fakeFolder.uploadState().children.count(), 1); + QVERIFY(fakeFolder.uploadState().children.first().name != chunkingId); + } + + + void testResumeServerDeletedChunks() { + + 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); + auto chunkingId = fakeFolder.uploadState().children.first().name; + + // Delete the chunks on the server + fakeFolder.uploadState().children.clear(); + QVERIFY(fakeFolder.syncOnce()); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size); + + // A different chunk id was used + QCOMPARE(fakeFolder.uploadState().children.count(), 1); + QVERIFY(fakeFolder.uploadState().children.first().name != chunkingId); + } + }; QTEST_GUILESS_MAIN(TestChunkingNG)