Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CFAPI: Handle cancelation of hydration requests #3010

Merged
merged 1 commit into from
Mar 18, 2021
Merged

CFAPI: Handle cancelation of hydration requests #3010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/libsync/propagatedownload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,15 @@ void GETFileJob::slotReadyRead()
}
}

void GETFileJob::cancel()
{
if (reply()->isRunning()) {
reply()->abort();
}

emit canceled();
}

void GETFileJob::onTimedOut()
{
qCWarning(lcGetJob) << "Timeout" << (reply() ? reply()->request().url() : path());
Expand Down
3 changes: 3 additions & 0 deletions src/libsync/propagatedownload.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ class GETFileJob : public AbstractNetworkJob
}
}

void cancel();

void newReplyHook(QNetworkReply *reply) override;

void setBandwidthManager(BandwidthManager *bwm);
Expand All @@ -108,6 +110,7 @@ class GETFileJob : public AbstractNetworkJob
void setExpectedContentLength(qint64 size) { _expectedContentLength = size; }

signals:
void canceled();
void finishedSignal();
void downloadProgress(qint64, qint64);
private slots:
Expand Down
20 changes: 19 additions & 1 deletion src/libsync/vfs/cfapi/cfapiwrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ void cfApiSendTransferInfo(const CF_CONNECTION_KEY &connectionKey, const CF_TRAN
}
}


void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const CF_CALLBACK_PARAMETERS *callbackParameters)
{
qDebug(lcCfApiWrapper) << "Fetch data callback called. File size:" << callbackInfo->FileSize.QuadPart;
Expand Down Expand Up @@ -181,8 +180,27 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const
}
}

void CALLBACK cfApiCancelFetchData(const CF_CALLBACK_INFO *callbackInfo, const CF_CALLBACK_PARAMETERS * /*callbackParameters*/)
{
const auto path = QString(QString::fromWCharArray(callbackInfo->VolumeDosName) + QString::fromWCharArray(callbackInfo->NormalizedPath));

qInfo(lcCfApiWrapper) << "Cancel fetch data of" << path;

auto vfs = reinterpret_cast<OCC::VfsCfApi *>(callbackInfo->CallbackContext);
Q_ASSERT(vfs->metaObject()->className() == QByteArrayLiteral("OCC::VfsCfApi"));
const auto requestId = QString::number(callbackInfo->TransferKey.QuadPart, 16);

const auto invokeResult = QMetaObject::invokeMethod(
vfs, [=] { vfs->cancelHydration(requestId, path); }, Qt::QueuedConnection);
if (!invokeResult) {
qCritical(lcCfApiWrapper) << "Failed to cancel hydration for" << path << requestId;
}
}


CF_CALLBACK_REGISTRATION cfApiCallbacks[] = {
{ CF_CALLBACK_TYPE_FETCH_DATA, cfApiFetchDataCallback },
{ CF_CALLBACK_TYPE_CANCEL_FETCH_DATA, cfApiCancelFetchData },
CF_CALLBACK_REGISTRATION_END
};

Expand Down
26 changes: 26 additions & 0 deletions src/libsync/vfs/cfapi/hydrationjob.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,15 @@ void OCC::HydrationJob::start()
connect(_server, &QLocalServer::newConnection, this, &HydrationJob::onNewConnection);
}

void OCC::HydrationJob::cancel()
{
if (!_job) {
return;
}

_job->cancel();
}

void OCC::HydrationJob::emitFinished(Status status)
{
_status = status;
Expand All @@ -131,6 +140,16 @@ void OCC::HydrationJob::emitFinished(Status status)
}
}

void OCC::HydrationJob::emitCanceled()
{
connect(_socket, &QLocalSocket::disconnected, this, [=] {
_socket->close();
});
_socket->disconnectFromServer();

emit canceled(this);
}

void OCC::HydrationJob::onNewConnection()
{
Q_ASSERT(!_socket);
Expand All @@ -140,9 +159,16 @@ void OCC::HydrationJob::onNewConnection()
_socket = _server->nextPendingConnection();
_job = new GETFileJob(_account, _remotePath + _folderPath, _socket, {}, {}, 0, this);
connect(_job, &GETFileJob::finishedSignal, this, &HydrationJob::onGetFinished);
connect(_job, &GETFileJob::canceled, this, &HydrationJob::onGetCanceled);
_job->start();
}

void OCC::HydrationJob::onGetCanceled()
{
qCInfo(lcHydration) << "GETFileJob canceled" << _requestId << _folderPath << _job->reply()->error();
emitCanceled();
}

void OCC::HydrationJob::onGetFinished()
{
qCInfo(lcHydration) << "GETFileJob finished" << _requestId << _folderPath << _job->reply()->error();
Expand Down
4 changes: 4 additions & 0 deletions src/libsync/vfs/cfapi/hydrationjob.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,19 @@ class OWNCLOUDSYNC_EXPORT HydrationJob : public QObject
Status status() const;

void start();
void cancel();

signals:
void finished(HydrationJob *job);
void canceled(HydrationJob *job);

private:
void emitFinished(Status status);
void emitCanceled();

void onNewConnection();
void onGetFinished();
void onGetCanceled();

AccountPtr _account;
QString _remotePath;
Expand Down
30 changes: 30 additions & 0 deletions src/libsync/vfs/cfapi/vfs_cfapi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,19 @@ Vfs::AvailabilityResult VfsCfApi::availability(const QString &folderPath)
return AvailabilityError::NoSuchItem;
}

void VfsCfApi::cancelHydration(const QString &requestId, const QString & /*path*/)
{
// Find matching hydration job for request id
const auto hydrationJobsIter = std::find_if(d->hydrationJobs.cbegin(), d->hydrationJobs.cend(), [&](const HydrationJob *job) {
return job->requestId() == requestId;
});

// If found, cancel it
if (hydrationJobsIter != d->hydrationJobs.cend()) {
(*hydrationJobsIter)->cancel();
}
}

void VfsCfApi::requestHydration(const QString &requestId, const QString &path)
{
qCInfo(lcCfApi) << "Received request to hydrate" << path << requestId;
Expand Down Expand Up @@ -331,6 +344,7 @@ void VfsCfApi::scheduleHydrationJob(const QString &requestId, const QString &fol
job->setRequestId(requestId);
job->setFolderPath(folderPath);
connect(job, &HydrationJob::finished, this, &VfsCfApi::onHydrationJobFinished);
connect(job, &HydrationJob::canceled, this, &VfsCfApi::onHydrationJobCanceled);
d->hydrationJobs << job;
job->start();
emit hydrationRequestReady(requestId);
Expand All @@ -347,6 +361,22 @@ void VfsCfApi::onHydrationJobFinished(HydrationJob *job)
}
}

void VfsCfApi::onHydrationJobCanceled(HydrationJob *job)
{
const auto folderPath = job->localPath();
const auto folderRelativePath = job->folderPath();

// Remove placeholder file because there might be already pumped
// some data into it
QFile::remove(folderPath + folderRelativePath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is surprising. Isn't the data committed on disk by cfapi only when the request completes? I'd be surprised they'd go through all the trouble to prevent the sync engine from writing directly in the file itself to then leave inconsistent data behind...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you cancel a request and then look at the file size you can see that the file size is not zero bytes anymore. But I'm not sure if there is really already data pumped in. I thought it would be cleaner to just remove it and replace it with an empty placeholder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you cancel a request and then look at the file size you can see that the file size is not zero bytes anymore.

I think you're slightly confused here. It's already non-zero before the request. The placeholders expose the size of the file as reported by the server metadata even if the file isn't hydrated.

Please test again without that removal and db + placeholder mangling. I mean when the client has to do this kind of things sure, but if that's not required then it's better not too, it's always a risk of ending up with inconsistent data if something fails during the course of such actions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@er-vin Actually, I have noticed what @FlexW mentioned. There is this field called "Size on disk" on every file in Windows if you open its properties dialog. It is growing as the hydration is progressing and its size is zero at the beginning. If one cancels the download in the Progress dialog, the "Size on disk" does not drop to zero.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice that there's a different field in the properties dialog. Didn't spot it the last time.

So yeah I guess it needs that kill + recreate placeholder then... shame on that API, just makes no sense to not let us write to the files directly then. :-)


// Create a new placeholder file
SyncJournalFileRecord record;
params().journal->getFileRecord(folderRelativePath, &record);
const auto item = SyncFileItem::fromSyncJournalFileRecord(record);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's really required (see other comment above), then at least it should check the db read actually succeeded and the record is valid, otherwise you could have a malformed item here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@er-vin Okay but how should we recover if the item is malformed? Or should we check if the record is valid before the removal of the placeholder and if the record is invalid then just return early from the function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably best to do the db operation first. And leave the file in place if that fails. At least that'd avoid unwanted deletion on next sync.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@er-vin #3028

createPlaceholder(*item);
}

VfsCfApi::HydratationAndPinStates VfsCfApi::computeRecursiveHydrationAndPinStates(const QString &folderPath, const Optional<PinState> &basePinState)
{
Q_ASSERT(!folderPath.endsWith('/'));
Expand Down
3 changes: 3 additions & 0 deletions src/libsync/vfs/cfapi/vfs_cfapi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ class VfsCfApi : public Vfs
Optional<PinState> pinState(const QString &folderPath) override;
AvailabilityResult availability(const QString &folderPath) override;

void cancelHydration(const QString &requestId, const QString &path);

public slots:
void requestHydration(const QString &requestId, const QString &path);
void fileStatusChanged(const QString &systemFileName, SyncFileStatus fileStatus) override;
Expand All @@ -68,6 +70,7 @@ public slots:
private:
void scheduleHydrationJob(const QString &requestId, const QString &folderPath);
void onHydrationJobFinished(HydrationJob *job);
void onHydrationJobCanceled(HydrationJob *job);

struct HasHydratedDehydrated {
bool hasHydrated = false;
Expand Down