VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always. #3390

allexzander · 2021-06-01T18:02:27Z

Signed-off-by: allexzander blackslayer4@gmail.com

This should fix #3318

FlexW · 2021-06-01T18:34:31Z

src/common/filesystembase.cpp

@@ -480,6 +480,11 @@ bool FileSystem::isLnkFile(const QString &filename)
    return filename.endsWith(QLatin1String(".lnk"));
 }

+bool FileSystem::isExcludeFile(const QString &filename)
+{
+    return filename.endsWith(QLatin1String(".sync-exclude.lst"));


I guess @mgallien will say better use QStringLiteral :)

You may also want to add exclude.lst as a exclude file. See src/libsync/conifgfile.cpp:362. Also it might be a good idea to create a named constant for these filenames.

QStringLiteral

@FlexW

Goot catch. It's a leftover. Gonna change it.

@FlexW

You may also want to add exclude.lst as a exclude file. See src/libsync/conifgfile.cpp:362.

Good idea.

@FlexW

Also it might be a good idea to create a named constant for these filenames.

Could do, but, where should I put that constant?

@allexzander How about src/csync/csync_exclude.h? May not be optimal but thats were it originally comes from I guess. src/common/filesystembase.h might also be a fit.

@FlexW I've checked both suggestions and neither suits well. src/common/filesystembase.h is not really a good choice because it is a helper file that is not supposed to store implementation details of the sync engine. src/csync/csync_exclude.h is also not the best place considering I will have then to import a part of sync engine into a helper class src/common/filesystembase.h which is in a common folder. I don't like having cross-dependencies. What I could do is - to add a separate header into a common folder. That header would then contain all the common constants. The problem is, however, I already have that header in another PR and don't want to create conflicts later. So, what I am thinking is - I should keep these file names hard-coded for now, and then, factor them out into a separate common header later in a separate PR.

@allexzander At least you put sync engine detail functions into src/common/filesystembase.h with isExclude(). So I don't see why adding these constants here might be not a fit :)

@FlexW You mean just adding constants into filesystembase and then using them just there? I've been thinking about something more common, like, having a header file that I can include into filesystembase and other places where this same .sync-exclude.lst is encountered, to make sure we have these constants all in one place.

@allexzander No meant that you can use the constants everywhere. Because isExclude() will also be used everywhere right?

mgallien · 2021-06-02T08:11:53Z

/rebase

allexzander · 2021-06-02T09:40:49Z

/rebase

@mgallien
Were you going to merge it?

mgallien · 2021-06-02T09:42:00Z

/rebase

@mgallien
Were you going to merge it?

not yet bu the debian part of the pipeline had failed hence why I did that to get a clean state

mgallien

So, if I understand correctly we are letting some files be synced within the sync directory without making them placeholders because we want to have them always there.
Will not that cause problems with other parts of CfApi or sync engine ?
Do we already have cases where we do that ?

allexzander · 2021-06-03T11:49:06Z

@mgallien

So, if I understand correctly we are letting some files be synced within the sync directory without making them placeholders because we want to have them always there.

Yes. In fact, we not just always want them always there because we don't want them to be virtual, it is more of a way sync engine works. When we request dehydration, those .sync-exclude.lst files are marked for dehydration, but, during the sync, it is required that they are on disk and they are attempted to get opened and parsed during the sync. This brings us to the infinite loop - we want to dehydrate a .sync-exclude.lst, but, at the same time, we want them to be hydrated. This leads to an app freezing.

Do we already have cases where we do that ?

Not that I am aware of. We have some files that we exclude from sync completely though due to VFS. We always exclude .lnk files, but, we can't exclude .lst files as they are affecting the folders' configuration in terms of files excluded from sync, so, we can only make them non-virtual, and we can't exclude them completely.

Will not that cause problems with other parts of CfApi or sync engine ?

Hard to say.

mgallien · 2021-06-07T14:46:57Z

src/common/filesystembase.cpp

@@ -480,6 +480,11 @@ bool FileSystem::isLnkFile(const QString &filename)
    return filename.endsWith(QLatin1String(".lnk"));
 }

+bool FileSystem::isExcludeFile(const QString &filename)
+{
+    return filename.endsWith(QStringLiteral("sync-exclude.lst")) || filename.endsWith(QStringLiteral("exclude.lst"));


I guess I do not understand why endsWith instead of an equal comparison ?
we are going to accept names we may not want

I guess I do not understand why endsWith instead of an equal comparison ?
we are going to accept names we may not want

@mgallien My plan was to also catch nested sync-exclude.lst, but, I will remove it. If someone reopens this issue later with nested sync-exclude.lst causing problems, I will then just bring endsWith back.

In that case, you can do the comparison such that you are sure not to catch filenames like test.exclude.lst but accept test/.exclude.lst or .exclude.lst

or if I did not understood correctly accept:

.sync-eclude.lst

test/.sync-exclude.lst

but refuse

test.sync-exclude.lst

test/rover.sync-exclude.lst

@mgallien makes sense, will do that

mgallien · 2021-06-08T09:24:50Z

/rebase

Signed-off-by: allexzander <blackslayer4@gmail.com>

nextcloud-desktop-bot · 2021-06-08T09:29:54Z

AppImage file: Nextcloud-PR-3390-d6ddf595f8d7d3b3995ed784ca383e9c8cc789fa-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.

allexzander · 2021-06-08T10:25:12Z

/backport to stable-3.2

allexzander force-pushed the bugfix/syncexclude-dehydration-freeze branch from 20f8fe5 to d27c906 Compare June 1, 2021 18:09

FlexW reviewed Jun 1, 2021

View reviewed changes

allexzander requested review from mgallien and camilasan June 2, 2021 08:06

mgallien requested a review from er-vin June 2, 2021 08:10

github-actions bot force-pushed the bugfix/syncexclude-dehydration-freeze branch from d27c906 to 524e4b2 Compare June 2, 2021 08:12

allexzander requested a review from FlexW June 2, 2021 10:45

allexzander force-pushed the bugfix/syncexclude-dehydration-freeze branch from 83d6e00 to db87558 Compare June 3, 2021 07:23

mgallien reviewed Jun 3, 2021

View reviewed changes

allexzander requested a review from mgallien June 7, 2021 11:47

FlexW approved these changes Jun 7, 2021

View reviewed changes

mgallien added the 2. to review label Jun 7, 2021

mgallien reviewed Jun 7, 2021

View reviewed changes

allexzander force-pushed the bugfix/syncexclude-dehydration-freeze branch from de99c5b to 3957d35 Compare June 8, 2021 06:45

allexzander requested a review from mgallien June 8, 2021 07:20

mgallien approved these changes Jun 8, 2021

View reviewed changes

allexzander added 4 commits June 8, 2021 09:25

VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always.

bb55232

Signed-off-by: allexzander <blackslayer4@gmail.com>

Fix review comments. Also consider exclude.lst.

b150de9

Signed-off-by: allexzander <blackslayer4@gmail.com>

Fix review comments.

5dd310c

Signed-off-by: allexzander <blackslayer4@gmail.com>

Fix review comments.

d6ddf59

Signed-off-by: allexzander <blackslayer4@gmail.com>

github-actions bot force-pushed the bugfix/syncexclude-dehydration-freeze branch from c73f9ec to d6ddf59 Compare June 8, 2021 09:25

allexzander merged commit c59e3c4 into master Jun 8, 2021

allexzander deleted the bugfix/syncexclude-dehydration-freeze branch June 8, 2021 09:44

backportbot-nextcloud bot added the backport-request label Jun 8, 2021

backportbot-nextcloud bot mentioned this pull request Jun 8, 2021

[stable-3.2] VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always. #3415

Merged

backportbot-nextcloud bot removed the backport-request label Jun 8, 2021

FlexW mentioned this pull request Jun 9, 2021

[VFS] Windows client hangs when you "make available offline" the .sync_* files #3417

Closed

claell mentioned this pull request Jun 29, 2021

Windows explorer frequently hangs #3451

Open

allexzander mentioned this pull request Jul 19, 2021

Windows 10 Client 3.2.1 keep crashing #3243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always. #3390

VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always. #3390

allexzander commented Jun 1, 2021

FlexW Jun 1, 2021

allexzander Jun 2, 2021

allexzander Jun 2, 2021

allexzander Jun 2, 2021

FlexW Jun 2, 2021

allexzander Jun 3, 2021

FlexW Jun 7, 2021

allexzander Jun 8, 2021

FlexW Jun 8, 2021

mgallien commented Jun 2, 2021

allexzander commented Jun 2, 2021

mgallien commented Jun 2, 2021

mgallien left a comment

allexzander commented Jun 3, 2021

mgallien Jun 7, 2021

allexzander Jun 7, 2021

mgallien Jun 8, 2021

mgallien Jun 8, 2021

allexzander Jun 8, 2021

mgallien commented Jun 8, 2021

nextcloud-desktop-bot commented Jun 8, 2021

allexzander commented Jun 8, 2021

VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always. #3390

VFS freeze fix. Treat .sync-exclude.lst as a non-virtual file always. #3390

Conversation

allexzander commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgallien commented Jun 2, 2021

allexzander commented Jun 2, 2021

mgallien commented Jun 2, 2021

mgallien left a comment

Choose a reason for hiding this comment

allexzander commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgallien commented Jun 8, 2021

nextcloud-desktop-bot commented Jun 8, 2021

allexzander commented Jun 8, 2021