From: Kevin Ottens Date: Tue, 29 Dec 2020 17:53:00 +0000 (+0100) Subject: Ensure we properly cancel hydration on server errors X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~447^2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=46a5bd6b253751db1a5db9f2603cd3724de4fb6a;p=nextcloud-desktop.git Ensure we properly cancel hydration on server errors Signed-off-by: Kevin Ottens --- diff --git a/src/libsync/vfs/cfapi/cfapiwrapper.cpp b/src/libsync/vfs/cfapi/cfapiwrapper.cpp index 90ace52be..62ffcffa1 100644 --- a/src/libsync/vfs/cfapi/cfapiwrapper.cpp +++ b/src/libsync/vfs/cfapi/cfapiwrapper.cpp @@ -15,6 +15,7 @@ #include "cfapiwrapper.h" #include "common/utility.h" +#include "hydrationjob.h" #include "vfs_cfapi.h" #include @@ -106,6 +107,7 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const } }); loop.exec(); + QObject::disconnect(vfs, nullptr, &loop, nullptr); qCInfo(lcCfApiWrapper) << "VFS replied for hydration of" << path << requestId << "status was:" << hydrationRequestResult; if (!hydrationRequestResult) { @@ -123,18 +125,31 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const } qint64 offset = 0; - while (socket.waitForReadyRead()) { + + QObject::connect(&socket, &QLocalSocket::readyRead, &loop, [&] { auto data = socket.readAll(); if (data.isEmpty()) { qCWarning(lcCfApiWrapper) << "Unexpected empty data received" << requestId; sendTransferError(); - break; + loop.quit(); + return; } sendTransferInfo(data, offset); offset += data.length(); - } + }); + + QObject::connect(vfs, &OCC::VfsCfApi::hydrationRequestFinished, &loop, [&](const QString &id, int s) { + if (requestId == id) { + const auto status = static_cast(s); + qCInfo(lcCfApiWrapper) << "Hydration done for" << path << requestId << status; + if (status != OCC::HydrationJob::Success) { + sendTransferError(); + } + loop.quit(); + } + }); - qCInfo(lcCfApiWrapper) << "Hydration done for" << path << requestId; + loop.exec(); } CF_CALLBACK_REGISTRATION cfApiCallbacks[] = { diff --git a/src/libsync/vfs/cfapi/hydrationjob.cpp b/src/libsync/vfs/cfapi/hydrationjob.cpp index 48a67951c..05af54e47 100644 --- a/src/libsync/vfs/cfapi/hydrationjob.cpp +++ b/src/libsync/vfs/cfapi/hydrationjob.cpp @@ -88,6 +88,11 @@ void OCC::HydrationJob::setFolderPath(const QString &folderPath) _folderPath = folderPath; } +OCC::HydrationJob::Status OCC::HydrationJob::status() const +{ + return _status; +} + void OCC::HydrationJob::start() { Q_ASSERT(_account); @@ -103,7 +108,7 @@ void OCC::HydrationJob::start() const auto listenResult = _server->listen(_requestId); if (!listenResult) { qCCritical(lcHydration) << "Couldn't get server to listen" << _requestId << _localPath << _folderPath; - emitFinished(); + emitFinished(Error); return; } @@ -111,13 +116,19 @@ void OCC::HydrationJob::start() connect(_server, &QLocalServer::newConnection, this, &HydrationJob::onNewConnection); } -void OCC::HydrationJob::emitFinished() +void OCC::HydrationJob::emitFinished(Status status) { - _socket->disconnectFromServer(); - connect(_socket, &QLocalSocket::disconnected, this, [=]{ + _status = status; + if (status == Success) { + _socket->disconnectFromServer(); + connect(_socket, &QLocalSocket::disconnected, this, [=]{ + _socket->close(); + emit finished(this); + }); + } else { _socket->close(); emit finished(this); - }); + } } void OCC::HydrationJob::onNewConnection() @@ -134,10 +145,10 @@ void OCC::HydrationJob::onNewConnection() void OCC::HydrationJob::onGetFinished() { - qCInfo(lcHydration) << "GETFileJob finished" << _requestId << _folderPath << _job->errorStatus() << _job->errorString(); + qCInfo(lcHydration) << "GETFileJob finished" << _requestId << _folderPath << _job->reply()->error(); - if (_job->errorStatus() != SyncFileItem::NoStatus && _job->errorStatus() != SyncFileItem::Success) { - emitFinished(); + if (_job->reply()->error()) { + emitFinished(Error); return; } @@ -146,11 +157,11 @@ void OCC::HydrationJob::onGetFinished() Q_ASSERT(record.isValid()); if (!record.isValid()) { qCWarning(lcHydration) << "Couldn't find record to update after hydration" << _requestId << _folderPath; - emitFinished(); + emitFinished(Error); return; } record._type = ItemTypeFile; _journal->setFileRecord(record); - emitFinished(); + emitFinished(Success); } diff --git a/src/libsync/vfs/cfapi/hydrationjob.h b/src/libsync/vfs/cfapi/hydrationjob.h index 6d2be8251..52709abe3 100644 --- a/src/libsync/vfs/cfapi/hydrationjob.h +++ b/src/libsync/vfs/cfapi/hydrationjob.h @@ -28,6 +28,12 @@ class OWNCLOUDSYNC_EXPORT HydrationJob : public QObject { Q_OBJECT public: + enum Status { + Success = 0, + Error, + }; + Q_ENUM(Status) + explicit HydrationJob(QObject *parent = nullptr); AccountPtr account() const; @@ -48,13 +54,15 @@ public: QString folderPath() const; void setFolderPath(const QString &folderPath); + Status status() const; + void start(); signals: void finished(HydrationJob *job); private: - void emitFinished(); + void emitFinished(Status status); void onNewConnection(); void onGetFinished(); @@ -70,6 +78,7 @@ private: QLocalServer *_server = nullptr; QLocalSocket *_socket = nullptr; GETFileJob *_job = nullptr; + Status _status = Success; }; } // namespace OCC diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.cpp b/src/libsync/vfs/cfapi/vfs_cfapi.cpp index 57776231b..4a73ad968 100644 --- a/src/libsync/vfs/cfapi/vfs_cfapi.cpp +++ b/src/libsync/vfs/cfapi/vfs_cfapi.cpp @@ -331,6 +331,8 @@ void VfsCfApi::scheduleHydrationJob(const QString &requestId, const QString &fol void VfsCfApi::onHydrationJobFinished(HydrationJob *job) { Q_ASSERT(d->hydrationJobs.contains(job)); + qCInfo(lcCfApi) << "Hydration job finished" << job->requestId() << job->folderPath() << job->status(); + emit hydrationRequestFinished(job->requestId(), job->status()); d->hydrationJobs.removeAll(job); if (d->hydrationJobs.isEmpty()) { emit doneHydrating(); diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.h b/src/libsync/vfs/cfapi/vfs_cfapi.h index 58ac88ea9..e4a4d0074 100644 --- a/src/libsync/vfs/cfapi/vfs_cfapi.h +++ b/src/libsync/vfs/cfapi/vfs_cfapi.h @@ -60,6 +60,7 @@ public slots: signals: void hydrationRequestReady(const QString &requestId); void hydrationRequestFailed(const QString &requestId); + void hydrationRequestFinished(const QString &requestId, int status); protected: void startImpl(const VfsSetupParams ¶ms) override; diff --git a/test/testsynccfapi.cpp b/test/testsynccfapi.cpp index 1d01c2acd..30634b1a2 100644 --- a/test/testsynccfapi.cpp +++ b/test/testsynccfapi.cpp @@ -35,6 +35,12 @@ using namespace OCC::CfApiWrapper; using namespace OCC; +enum ErrorKind : int { + NoError = 0, + // Lower code are corresponding to HTTP error code + Timeout = 1000, +}; + void setPinState(const QString &path, PinState state, cfapi::SetPinRecurseMode mode) { Q_ASSERT(mode == cfapi::Recurse || mode == cfapi::NoRecurse); @@ -1098,8 +1104,23 @@ private slots: CFVERIFY_VIRTUAL(fakeFolder, "local/file1"); } + void testOpeningOnlineFileTriggersDownload_data() + { + QTest::addColumn("errorKind"); + QTest::newRow("no error") << static_cast(NoError); + QTest::newRow("400") << 400; + QTest::newRow("401") << 401; + QTest::newRow("403") << 403; + QTest::newRow("404") << 404; + QTest::newRow("500") << 500; + QTest::newRow("503") << 503; + QTest::newRow("Timeout") << static_cast(Timeout); + } + void testOpeningOnlineFileTriggersDownload() { + QFETCH(int, errorKind); + FakeFolder fakeFolder{ FileInfo() }; setupVfs(fakeFolder); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); @@ -1116,6 +1137,21 @@ private slots: CFVERIFY_VIRTUAL(fakeFolder, "online/sub/file1"); + // Setup error case if needed + if (errorKind == Timeout) { + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) -> QNetworkReply * { + if (req.url().path().endsWith("online/sub/file1")) { + return new FakeHangingReply(op, req, this); + } + return nullptr; + }); + } else if (errorKind != NoError) { + fakeFolder.serverErrorPaths().append("online/sub/file1", errorKind); + } + + // So the test that test timeout finishes fast + QScopedValueRollback setHttpTimeout(AbstractNetworkJob::httpTimeout, errorKind == Timeout ? 1 : 10000); + // Simulate another process requesting the open QEventLoop loop; bool openResult = false; @@ -1130,14 +1166,22 @@ private slots: loop.exec(); t.join(); - CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1"); + if (errorKind == NoError) { + CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1"); + } else { + CFVERIFY_VIRTUAL(fakeFolder, "online/sub/file1"); + } // Nothing should change ItemCompletedSpy completeSpy(fakeFolder); QVERIFY(fakeFolder.syncOnce()); QVERIFY(completeSpy.isEmpty()); - CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1"); + if (errorKind == NoError) { + CFVERIFY_NONVIRTUAL(fakeFolder, "online/sub/file1"); + } else { + CFVERIFY_VIRTUAL(fakeFolder, "online/sub/file1"); + } } };