From 15f2b911d960dd88e86a388ccf1ced3eda0de4dd Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 31 Oct 2016 17:44:00 +0100 Subject: [PATCH] ChunkingNG: remove stale files when resuming --- src/libsync/propagateremotedelete.cpp | 5 +- src/libsync/propagateremotedelete.h | 2 + src/libsync/propagateupload.h | 2 + src/libsync/propagateuploadng.cpp | 68 ++++++++++++++++++++++++--- test/syncenginetestutils.h | 17 ++++--- test/testchunkingng.cpp | 41 +++++++++++++++- 6 files changed, 119 insertions(+), 16 deletions(-) diff --git a/src/libsync/propagateremotedelete.cpp b/src/libsync/propagateremotedelete.cpp index 30b06fe41..9be19c309 100644 --- a/src/libsync/propagateremotedelete.cpp +++ b/src/libsync/propagateremotedelete.cpp @@ -22,11 +22,14 @@ DeleteJob::DeleteJob(AccountPtr account, const QString& path, QObject* parent) : AbstractNetworkJob(account, path, parent) { } +DeleteJob::DeleteJob(AccountPtr account, const QUrl& url, QObject* parent) + : AbstractNetworkJob(account, QString(), parent), _url(url) +{ } void DeleteJob::start() { QNetworkRequest req; - setReply(davRequest("DELETE", path(), req)); + setReply(_url.isValid() ? davRequest("DELETE", _url, req) : davRequest("DELETE", path(), req)); setupConnections(reply()); if( reply()->error() != QNetworkReply::NoError ) { diff --git a/src/libsync/propagateremotedelete.h b/src/libsync/propagateremotedelete.h index 0473042c8..44f4ebd3b 100644 --- a/src/libsync/propagateremotedelete.h +++ b/src/libsync/propagateremotedelete.h @@ -24,8 +24,10 @@ namespace OCC { */ class DeleteJob : public AbstractNetworkJob { Q_OBJECT + QUrl _url; // Only used if the constructor taking a url is taken. public: explicit DeleteJob(AccountPtr account, const QString& path, QObject* parent = 0); + explicit DeleteJob(AccountPtr account, const QUrl& url, QObject* parent = 0); void start() Q_DECL_OVERRIDE; bool finished() Q_DECL_OVERRIDE; diff --git a/src/libsync/propagateupload.h b/src/libsync/propagateupload.h index e7f88f169..bac3e112b 100644 --- a/src/libsync/propagateupload.h +++ b/src/libsync/propagateupload.h @@ -289,6 +289,7 @@ private: quint64 _sent; /// amount of data (bytes) that was already sent uint _transferId; /// transfer id (part of the url) int _currentChunk; /// Id of the next chunk that will be sent + bool _removeJobError; /// If not null, there was an error removing the job // Map chunk number with its size from the PROPFIND on resume. // (Only used from slotPropfindIterate/slotPropfindFinished because the LsColJob use signals to report data.) @@ -313,6 +314,7 @@ private slots: void slotPropfindFinished(); void slotPropfindFinishedWithError(); void slotPropfindIterate(const QString &name, const QMap &properties); + void slotDeleteJobFinished(); void slotMkColFinished(QNetworkReply::NetworkError); void slotPutFinished(); void slotMoveJobFinished(); diff --git a/src/libsync/propagateuploadng.cpp b/src/libsync/propagateuploadng.cpp index abfd8257c..5cddc22ac 100644 --- a/src/libsync/propagateuploadng.cpp +++ b/src/libsync/propagateuploadng.cpp @@ -24,6 +24,8 @@ #include "propagatorjobs.h" #include "syncengine.h" #include "propagateremotemove.h" +#include "propagateremotedelete.h" + #include #include @@ -56,10 +58,13 @@ QUrl PropagateUploadFileNG::chunkUrl(int chunk) startNewUpload() <-+ +----------------------------\ | | | \ MKCOL + slotPropfindFinishedWithError() slotPropfindFinished() - | | - slotMkColFinished() | - | | - +-----+-------------------------------------------------------+ + | Is there stale files to remove? + slotMkColFinished() | | + | no yes + | | | + | | DeleteJob + | | | + +-----+<------------------------------------------------------+<--- slotDeleteJobFinished() | +----> startNextChunk() ---finished? --+ ^ | | @@ -119,12 +124,29 @@ void PropagateUploadFileNG::slotPropfindFinished() _sent = 0; while (_serverChunks.contains(_currentChunk)) { _sent += _serverChunks[_currentChunk]; + _serverChunks.remove(_currentChunk); ++_currentChunk; } - // FIXME: we should make sure that if there is a "hole" and then a few more chunks, on the server - // we should remove the later chunks. Otherwise when we do dynamic chunk sizing, we may end up - // with corruptions if there are too many chunks, or if we abort and there are still stale chunks. + qDebug() << "Resuming "<< _item->_file << " from chunk " << _currentChunk << "; sent ="<< _sent; + + if (!_serverChunks.isEmpty()) { + qDebug() << "To Delete" << _serverChunks; + _propagator->_activeJobList.append(this); + _removeJobError = false; + + // Make sure that if there is a "hole" and then a few more chunks, on the server + // we should remove the later chunks. Otherwise when we do dynamic chunk sizing, we may end up + // with corruptions if there are too many chunks, or if we abort and there are still stale chunks. + for (auto it = _serverChunks.begin(); it != _serverChunks.end(); ++it) { + auto job = new DeleteJob(_propagator->account(), Utility::concatUrlPath(chunkUrl(), QString::number(it.key())), this); + QObject::connect(job, SIGNAL(finishedSignal()), this, SLOT(slotDeleteJobFinished())); + _jobs.append(job); + job->start(); + } + return; + } + startNextChunk(); } @@ -144,6 +166,38 @@ void PropagateUploadFileNG::slotPropfindFinishedWithError() startNewUpload(); } +void PropagateUploadFileNG::slotDeleteJobFinished() +{ + auto job = qobject_cast(sender()); + Q_ASSERT(job); + _jobs.remove(_jobs.indexOf(job)); + + QNetworkReply::NetworkError err = job->reply()->error(); + if (err != QNetworkReply::NoError && err != QNetworkReply::ContentNotFoundError) { + const int httpStatus = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + SyncFileItem::Status status = classifyError(err, httpStatus); + if (status == SyncFileItem::FatalError) { + abortWithError(status, job->errorString()); + return; + } else { + qWarning() << "DeleteJob errored out" << job->errorString() << job->reply()->url(); + _removeJobError = true; + // Let the other jobs finish + } + } + + if (_jobs.isEmpty()) { + _propagator->_activeJobList.removeOne(this); + if (_removeJobError) { + // There was an error removing some files, just start over + startNewUpload(); + } else { + startNextChunk(); + } + } +} + + void PropagateUploadFileNG::startNewUpload() { diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index 51d7ce421..cbcb5fab2 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -297,6 +297,12 @@ public: setOperation(op); open(QIODevice::ReadOnly); + QString fileName = getFilePathFromUrl(request.url()); + Q_ASSERT(!fileName.isNull()); // for root, it should be empty + const FileInfo *fileInfo = remoteRootFileInfo.find(fileName); + Q_ASSERT(fileInfo); + QString prefix = request.url().path().left(request.url().path().size() - fileName.size()); + // Don't care about the request and just return a full propfind const QString davUri{QStringLiteral("DAV:")}; const QString ocUri{QStringLiteral("http://owncloud.org/ns")}; @@ -310,7 +316,7 @@ public: auto writeFileResponse = [&](const FileInfo &fileInfo) { xml.writeStartElement(davUri, QStringLiteral("response")); - xml.writeTextElement(davUri, QStringLiteral("href"), "/owncloud/remote.php/webdav/" + fileInfo.path()); + xml.writeTextElement(davUri, QStringLiteral("href"), prefix + fileInfo.path()); xml.writeStartElement(davUri, QStringLiteral("propstat")); xml.writeStartElement(davUri, QStringLiteral("prop")); @@ -334,11 +340,6 @@ public: xml.writeEndElement(); // response }; - QString fileName = getFilePathFromUrl(request.url()); - Q_ASSERT(!fileName.isNull()); // for root, it should be empty - const FileInfo *fileInfo = remoteRootFileInfo.find(fileName); - Q_ASSERT(fileInfo); - writeFileResponse(*fileInfo); foreach(const FileInfo &childFileInfo, fileInfo->children) writeFileResponse(childFileInfo); @@ -400,6 +401,7 @@ public: } Q_INVOKABLE void respond() { + emit uploadProgress(fileInfo->size, fileInfo->size); setRawHeader("OC-ETag", fileInfo->etag.toLatin1()); setRawHeader("ETag", fileInfo->etag.toLatin1()); setRawHeader("X-OC-MTime", "accepted"); // Prevents Q_ASSERT(!_runningNow) since we'll call PropagateItemJob::done twice in that case. @@ -583,6 +585,7 @@ public: } while(true); Q_ASSERT(count > 1); // There should be at least two chunks, otherwise why would we use chunking? + QCOMPARE(sourceFolder->children.count(), count); // There should not be holes or extra files QString fileName = getFilePathFromUrl(QUrl::fromEncoded(request.rawHeader("Destination"))); Q_ASSERT(!fileName.isEmpty()); @@ -750,7 +753,7 @@ public: } FileInfo currentRemoteState() { return _fakeQnam->currentRemoteState(); } - FileInfo uploadState() { return _fakeQnam->uploadState(); } + FileInfo &uploadState() { return _fakeQnam->uploadState(); } QStringList &serverErrorPaths() { return _fakeQnam->errorPaths(); } diff --git a/test/testchunkingng.cpp b/test/testchunkingng.cpp index 474aa8f5c..21225c210 100644 --- a/test/testchunkingng.cpp +++ b/test/testchunkingng.cpp @@ -25,7 +25,7 @@ private slots: QVERIFY(fakeFolder.syncOnce()); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); QCOMPARE(fakeFolder.uploadState().children.count(), 1); // the transfer was done with chunking - QCOMPARE(fakeFolder.currentLocalState().find("A/a0")->size, size); + QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size); // Check that another upload of the same file also work. fakeFolder.localModifier().appendByte("A/a0"); @@ -33,6 +33,45 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); QCOMPARE(fakeFolder.uploadState().children.count(), 2); // the transfer was done with chunking } + + + void testResume () { + + 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; })); + + + // 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.uploadState().children.count(), 1); // The same chunk id was re-used + QCOMPARE(fakeFolder.currentRemoteState().find("A/a0")->size, size); + } }; QTEST_GUILESS_MAIN(TestChunkingNG) -- 2.39.5