status wrong for directories when using vfs and nextcloud desktop 3.2.0 #3170

mgallien · 2021-04-19T13:11:31Z

mgallien · 2021-04-20T07:56:28Z

/rebase

src/libsync/vfs/cfapi/cfapiwrapper.cpp

er-vin · 2021-04-21T07:38:25Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -694,7 +700,7 @@ OCC::Result<void, QString> OCC::CfApiWrapper::convertToPlaceholder(const FileHan

    const auto fileIdentity = QString::fromUtf8(fileId).toStdWString();
    const auto fileIdentitySize = (fileIdentity.length() + 1) * sizeof(wchar_t);
-    const qint64 result = CfConvertToPlaceholder(handle.get(), fileIdentity.data(), sizeToDWORD(fileIdentitySize), CF_CONVERT_FLAG_NONE, nullptr, nullptr);
+    const qint64 result = CfConvertToPlaceholder(handle.get(), fileIdentity.data(), sizeToDWORD(fileIdentitySize), CF_CONVERT_FLAG_MARK_IN_SYNC, nullptr, nullptr);


That fills a bit heavy handed to me since it'll mark it sync indiscriminately all placeholders which I guess it might not always be the case... or my memory of that API gets fuzzier now. Isn't it something we'd rather deal with from VfsCfApi::fileStatusChanged?

Currently its implementation is empty because in my tests I couldn't find anything really wrong with having CF_CONVERT_FLAG_NONE and nothing more. But apparently I was looking at the wrong place.

As far as I understand, HydrationJob are used when some software (via the user) wants to access a file that has not been hydrated.

If the file is being downloaded because it has been pinned, we do not use the HydrationJob but we always go through CfConvertToPlaceholder.

I did some tests with the change and could not notice issues. I can try harder to find issues.

As far as I understand, HydrationJob are used when some software (via the user) wants to access a file that has not been hydrated.

Right and so this should handle the flag change for us I guess?

If the file is being downloaded because it has been pinned, we do not use the HydrationJob but we always go through CfConvertToPlaceholder.

Indeed. Is it all files or only for files which are unavailable? I admit I don't quite remember... That's also where I'm not sure if CF_CONVERT_FLAG_MARK_IN_SYNC is really appropriate in all cases. Because there are the transient states like "it needs a sync" and "it's syncing". I wonder how that plays along with those.

As you can tell I'm mostly brain dumping here. :-)

I did some tests with the change and could not notice issues. I can try harder to find issues.

I'd look around transient states mostly (makes them harder to test though). Might not be a problem in practice.

@er-vin Documentation for this API feels incomplete, really. From my tests on Matthiue's branch, this status icon's state behaves much better now.

As far as I understand, HydrationJob are used when some software (via the user) wants to access a file that has not been hydrated.

Right and so this should handle the flag change for us I guess?

And this part is showing the correct status. I had just needed to avoid having the progress bar reappear when the download is completed.

If the file is being downloaded because it has been pinned, we do not use the HydrationJob but we always go through CfConvertToPlaceholder.

Indeed. Is it all files or only for files which are unavailable? I admit I don't quite remember... That's also where I'm not sure if CF_CONVERT_FLAG_MARK_IN_SYNC is really appropriate in all cases. Because there are the transient states like "it needs a sync" and "it's syncing". I wonder how that plays along with those.

If I understand what happen, we have:

the user modifies the pinning state of some files that were cloud only (not hydrated)

our directory watcher detects that and we see that files have to be downloaded

we convert the plain old files into placeholders via CfConvertToPlaceholder

I suspect that indeed I have not tested enough what happen if a files is half downloaded. I suppose that we already have quite some code handling that given we are just using the pre-existing code path to do that. This is just after completeion that we have special handling of CfApi.

As you can tell I'm mostly brain dumping here. :-)

I did some tests with the change and could not notice issues. I can try harder to find issues.

I'd look around transient states mostly (makes them harder to test though). Might not be a problem in practice.

I will. Thanks for the suggestion

I did more tests and could only get an error when trying implicit hydration at the same time files where being downloaded by pinning.
At all time, it looks like file status were correct.

Unrelated note: When trying to open the same file twice, I got two download and the second one terminated with an error.

@mgallien Would be good to detect "this file is already being hydrated" and then - early-out from the second hydration attempt.

Implicit hydration of the same file two times is not triggering any error and is behaving like OneDrive. I believe we are OK there.

allexzander

LGTM. And, agree about Kevin's comment.

er-vin · 2021-04-21T10:03:06Z

@er-vin Documentation for this API feels incomplete, really.

Yes... We're badly spoiled with the Qt API documentation. ;-)

From my tests on Matthiue's branch, this status icon's state behaves much better now.

I don't doubt it, just pushing to make sure there's no annoying corner case lurking. They're easier to fix when the change is introduced than later on. <insert plot about cost of bug fix over age of bug here> :-)

allexzander · 2021-04-21T13:58:41Z

@er-vin

I don't doubt it, just pushing to make sure there's no annoying corner case lurking. They're easier to fix when the change is introduced than later on. :-)

This is also true, but, with this new CfAPI thing, I am afraid, this is not the last issue. We'd still need some feedback from users to see how things work out in this case of, rather poorly documented API. Not sure how to check the correctness of this state right now. But, surely, this part is not risky at all, since it's just the front-end part that does not affect the hydration/dehadration. And this is also the reason why it's hard to validate it. It's because this front-end part is in Windows' internals completely :(

er-vin · 2021-04-21T15:00:50Z

@er-vin

I don't doubt it, just pushing to make sure there's no annoying corner case lurking. They're easier to fix when the change is introduced than later on. :-)

This is also true, but, with this new CfAPI thing, I am afraid, this is not the last issue. We'd still need some feedback from users to see how things work out in this case of, rather poorly documented API. Not sure how to check the correctness of this state right now.

Unfortunately I have to agree. And that's why I like to limit platform specific code as much as possible, it often ends up like this otherwise. :-)

But, surely, this part is not risky at all, since it's just the front-end part that does not affect the hydration/dehadration.

Which in itself is "interesting" isn't it? The API pushes to claim it's synced while data is not there... 😞

And this is also the reason why it's hard to validate it. It's because this front-end part is in Windows' internals completely :(

Yes, there seem to be some lower level APIs which allow to drill into those states somehow... but making automated tests leveraging them is gruesome.

mgallien · 2021-04-26T07:41:36Z

So, when a file is being downloaded because it is being pinned and the user is also trying to open it (with implicit hydration being requested), we will get an abort on the file sync job. There is even a TODO comment to fix that. This code is still the same on the original repository where this is code is coming from. So something we should eventually fix but not now.

er-vin

LGTM

when using Cloud Filter API with enabled VFS on Windows, a progress bar stays visible for some time after hydration is completed. Not updating a last time the progress bar prevents that. Signed-off-by: Matthieu Gallien <matthieu_gallien@yahoo.fr>

files that get downloaded not through an hydration request need to be converted to placeholder sets the expected state when converting them to placeholder files #3082 Signed-off-by: Matthieu Gallien <matthieu_gallien@yahoo.fr>

nextcloud-desktop-bot · 2021-04-26T12:00:18Z

AppImage file: Nextcloud-PR-3170-419bd93dea208470647569e960a0b2ca95d7d6f5-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

mgallien · 2021-04-26T12:12:36Z

/backport to stable-3.2

mgallien force-pushed the bug/fixVfsStatusDelay branch from d387fb4 to 80e39fe Compare April 19, 2021 13:12

mgallien requested review from er-vin, FlexW, allexzander and camilasan April 19, 2021 13:12

github-actions bot force-pushed the bug/fixVfsStatusDelay branch from 80e39fe to b3c08ad Compare April 20, 2021 07:57

er-vin reviewed Apr 21, 2021

View reviewed changes

allexzander approved these changes Apr 21, 2021

View reviewed changes

mgallien force-pushed the bug/fixVfsStatusDelay branch from b3c08ad to d87a90f Compare April 21, 2021 08:20

mgallien force-pushed the bug/fixVfsStatusDelay branch from d87a90f to e1b3354 Compare April 23, 2021 07:45

er-vin reviewed Apr 26, 2021

View reviewed changes

mgallien added 2 commits April 26, 2021 13:54

mgallien force-pushed the bug/fixVfsStatusDelay branch from e1b3354 to 419bd93 Compare April 26, 2021 11:55

mgallien merged commit e9b3144 into master Apr 26, 2021

mgallien deleted the bug/fixVfsStatusDelay branch April 26, 2021 12:12

backportbot-nextcloud bot added the backport-request label Apr 26, 2021

backportbot-nextcloud bot mentioned this pull request Apr 26, 2021

[stable-3.2] status wrong for directories when using vfs and nextcloud desktop 3.2.0 #3213

Merged

backportbot-nextcloud bot removed the backport-request label Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

status wrong for directories when using vfs and nextcloud desktop 3.2.0 #3170

status wrong for directories when using vfs and nextcloud desktop 3.2.0 #3170

mgallien commented Apr 19, 2021

mgallien commented Apr 20, 2021

er-vin Apr 21, 2021

mgallien Apr 21, 2021 •

edited

Loading

er-vin Apr 21, 2021

allexzander Apr 21, 2021

mgallien Apr 21, 2021

mgallien Apr 23, 2021

allexzander Apr 24, 2021

mgallien Apr 26, 2021

allexzander left a comment

er-vin commented Apr 21, 2021 via email

allexzander commented Apr 21, 2021

er-vin commented Apr 21, 2021

mgallien commented Apr 26, 2021

er-vin left a comment

nextcloud-desktop-bot commented Apr 26, 2021

mgallien commented Apr 26, 2021

status wrong for directories when using vfs and nextcloud desktop 3.2.0 #3170

status wrong for directories when using vfs and nextcloud desktop 3.2.0 #3170

Conversation

mgallien commented Apr 19, 2021

mgallien commented Apr 20, 2021

er-vin Apr 21, 2021

Choose a reason for hiding this comment

mgallien Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

er-vin Apr 21, 2021

Choose a reason for hiding this comment

allexzander Apr 21, 2021

Choose a reason for hiding this comment

mgallien Apr 21, 2021

Choose a reason for hiding this comment

mgallien Apr 23, 2021

Choose a reason for hiding this comment

allexzander Apr 24, 2021

Choose a reason for hiding this comment

mgallien Apr 26, 2021

Choose a reason for hiding this comment

allexzander left a comment

Choose a reason for hiding this comment

er-vin commented Apr 21, 2021 via email

allexzander commented Apr 21, 2021

er-vin commented Apr 21, 2021

mgallien commented Apr 26, 2021

er-vin left a comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Apr 26, 2021

mgallien commented Apr 26, 2021

mgallien Apr 21, 2021 •

edited

Loading