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.

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

Avoid depth infinity propfind for e2ee #2572

Merged
merged 7 commits into from
Oct 21, 2020

Conversation

er-vin
Copy link
Member

@er-vin er-vin commented Oct 20, 2020

OK, so after all I'll go for changing the depth of the incriminated PROPFIND call and do the recursive traversal by hand (which means more roundtrips which is a bit unfortunate). I finally did this for several reasons:

  • we get the fix delivered faster in the hand of users;
  • even if we optimized it on the server to somehow complete, it'd still be potentially expensive;
  • also I found today this depth was used in other code paths than the first call when the sync starts which made things worse (and that was clearly the client at fault there so needed fixing anyway).

Fixes #2347
Fixes nextcloud/end_to_end_encryption#189

@er-vin er-vin added this to the Desktop 3.1 milestone Oct 20, 2020
@er-vin er-vin force-pushed the avoid_depth_infinity_propfind_for_e2ee branch 2 times, most recently from e254daa to 19eccc4 Compare October 20, 2020 15:18
Kevin Ottens added 7 commits October 21, 2020 10:00
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
If there's more than one job we need to unite the maps not simply overwrite
them.

Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
This way we avoid the expensive SQL query on the server at the price of
more round-trips since we're doing the recursive traversal by hand now.

Also it turns out this depth was used for all the other propfind calls
during sync when we want fresher information regarding a folder. This
was very inefficient in all cases and won't happen anymore.

Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
@er-vin er-vin force-pushed the avoid_depth_infinity_propfind_for_e2ee branch from 19eccc4 to 10cb417 Compare October 21, 2020 08:01
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-2572-10cb4170c7354c919a415f1082cf7bb0618ef988-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.

@er-vin er-vin merged commit 60da0ce into master Oct 21, 2020
@er-vin er-vin deleted the avoid_depth_infinity_propfind_for_e2ee branch October 21, 2020 08:16
@epikurist
Copy link

Thank you guys! Really appreciate the work. I have been waiting for 2,5 years to have the end-to-end encryption.

@er-vin
Copy link
Member Author

er-vin commented Oct 21, 2020

/backport to stable-3.0

@bcutter
Copy link

bcutter commented Oct 23, 2020

I tested v3.1.0.15934-daily-20201022.

Behavior changed:

After an initial sync after starting desktop client - which put server at 100 % load for about 30 seconds, desktop client stays in state „waiting - preparing to sync“ and will never re-sync (waited 15 minutes).
image

Switched back to latest v3.0.2 where one single sync progress takes 10 minutes while putting only one core to 100 %, but at least is syncing even it takes veeeery long.
image

Is this fix part of the daily I tested?

@kruthoff
Copy link

Same here. I'm testing the AppImage provided above. Sync scans folders and ends in waiting state. It doesn't work as expected.

@er-vin
Copy link
Member Author

er-vin commented Oct 26, 2020

Well, at least it doesn't drive the server crazy anymore which was the intention of working around that Depth parameter. It's clearly hitting something else now. Will need to go through the log exploration etc. dance again.

When doing so please make sure this issue isn't already reported, and if that's not the case please file a new one.

@bcutter
Copy link

bcutter commented Oct 30, 2020

Well, at least it doesn't drive the server crazy anymore which was the intention of working around that Depth parameter. It's clearly hitting something else now. Will need to go through the log exploration etc. dance again.

When doing so please make sure this issue isn't already reported, and if that's not the case please file a new one.

Ehm - but now it DOESN`T SYNC AT ALL (you realized that?) so I assume that´s not really an improvement.
Now as 3.0.3 with this "fix" is released you can expect more users crying - as least as soon as it´s released for auto-update in the client.

@er-vin
Copy link
Member Author

er-vin commented Nov 2, 2020

Well, at least it doesn't drive the server crazy anymore which was the intention of working around that Depth parameter. It's clearly hitting something else now. Will need to go through the log exploration etc. dance again.
When doing so please make sure this issue isn't already reported, and if that's not the case please file a new one.

Ehm - but now it DOESN`T SYNC AT ALL (you realized that?) so I assume that´s not really an improvement.

Well, it does sync for some. The part of the code I changed is behavior compatible with the old code but uses a depth 1 instead of depth infinity. I really think this is hitting something else now. There's been past reports of "getting stuck" before and after that fix so to me it's a different issue which needs being tackled as well. As mentioned on #1269 I'm interested in getting debug logs to figure it out but so far I'm not hitting it.

@bcutter
Copy link

bcutter commented Nov 15, 2020

Well, at least it doesn't drive the server crazy anymore which was the intention of working around that Depth parameter. It's clearly hitting something else now. Will need to go through the log exploration etc. dance again.
When doing so please make sure this issue isn't already reported, and if that's not the case please file a new one.

Ehm - but now it DOESN`T SYNC AT ALL (you realized that?) so I assume that´s not really an improvement.

Well, it does sync for some. The part of the code I changed is behavior compatible with the old code but uses a depth 1 instead of depth infinity. I really think this is hitting something else now. There's been past reports of "getting stuck" before and after that fix so to me it's a different issue which needs being tackled as well. As mentioned on #1269 I'm interested in getting debug logs to figure it out but so far I'm not hitting it.

How to provide client logs. Content of ".owncloudsync.log" seems not to be debug log level.

@er-vin
Copy link
Member Author

er-vin commented Nov 16, 2020

Nope that's not the one, but let me give some more details on #1269 about this

@hdlineage
Copy link

This is not fixed in 3.0.3 client. Enabling E2EE on the server side basically cripples the syncing process on windows desktop.

@bcutter
Copy link

bcutter commented Nov 28, 2020

This is not fixed in 3.0.3 client. Enabling E2EE on the server side basically cripples the syncing process on windows desktop.

One needs to provide client logs so @er-vin can take a look at it, see reference in his post above.

@MegaV0lt
Copy link

MegaV0lt commented Dec 6, 2020

Not fixed on linux. client still hangs

client linux 3.03

@MegaV0lt
Copy link

MegaV0lt commented Dec 6, 2020

@er-vin
Have made a debuglog. should i remov some sensitive data before upload?

@er-vin
Copy link
Member Author

er-vin commented Dec 7, 2020

@er-vin
Have made a debuglog. should i remov some sensitive data before upload?

Thanks for your efforts. We now know what's going on I think. Feel free to add the logs just in case it'd add something new. If you go for the file drop you don't have to remove sensitive data if you trust us enough to be responsible enough with it, otherwise please remove sensitive data first.

@victordariovera
Copy link

Here OSX Client v.3.0.3 (over nextcloud v. 19.0.3) no way to sync with E2E. Android client is ok but not on desktop. If I can ofuscate all the personal info in the client logs (a lot of metadata) I will attach it. (I don't know any tool to do it easy)

@er-vin
Copy link
Member Author

er-vin commented Dec 14, 2020

Alright, I don't like to do support on PRs, but since the chatter is still going on here. For the adventurous who are on Linux, it'd be useful if you could test the AppImage corresponding to #2700 (it's posted there). I think it should finally put the timeouts to rest.

Word of caution: note this is a massive PR, it might kill kittens, burn your house or... well... you get the idea. Use with caution, it should become 3.2 in March so we still have plenty of time to QA it but having early feedback is always useful.

Note it's not going to solve it for the crowd having issues removing E2EE folders. It's a completely different issue and will require it's on PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.0.3] Waiting for sync forever - not syncing [E2EE app bug] Enabled e2ee and now no clients are syncing
9 participants