CFAPI: Handle cancelation of hydration requests #3010
Conversation
|
|
||
| // Remove placeholder file because there might be already pumped | ||
| // some data into it | ||
| QFile::remove(folderPath + folderRelativePath); |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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. :-)
Thanks. I tried to find a way but did not have success so far. I'm open to ideas :) |
FlexW
force-pushed
the
feature/cfapi-handle-cancelation
branch
from
699081b to
2366153
Compare
FlexW
force-pushed
the
feature/cfapi-handle-cancelation
branch
from
faf11fd to
2a75a81
Compare
FlexW
force-pushed
the
feature/cfapi-handle-cancelation
branch
from
2a75a81 to
444b8b3
Compare
|
/rebase |
Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>
github-actions
bot
force-pushed
the
feature/cfapi-handle-cancelation
branch
from
444b8b3 to
9bf5b5c
Compare
|
AppImage file: Nextcloud-PR-3010-9bf5b5c7ba3ec59f86a7f51bca55bbd63d9f402a-x86_64.AppImage |
| // Create a new placeholder file | ||
| SyncJournalFileRecord record; | ||
| params().journal->getFileRecord(folderRelativePath, &record); | ||
| const auto item = SyncFileItem::fromSyncJournalFileRecord(record); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com