-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Deletion of several folders or files fails when using external storages. #28779
Comments
Does it happen on stable10 too ? |
The reference issue was tested in stable10, so it happens there as well. |
And likely doesn't happen on the home storage ? Does it happen with "local" as external storage ? |
Happens also using local external storage. (Using master) |
but does not happen for a home storage ? |
Reproduced locally with stable10 and SFTP |
Only happens using external storage. |
@SergioBertolinSG I've now tried with locking with redis and the problem did not appear. I wonder if some deadlock or something is happening with DB based locking. |
With DB locking:
|
There are scanner entries for the locks. I tried setting the ext storage mode to "Never" check for changes, still the locking errors appear. Well, now it's worse: I get 403 for all three calls:
|
For the case with "check updates" enabled which matches this original report, what I observe when deleting three folders "a", "b" and "c" on external storage: The DELETE call for "b" apparently also shortly sets a shared lock on "a", "b" and also "c" instead of just "b". Not sure why, could be the scanner checking their Now the first thing that comes to mind is the RetryLockingWrapper attempt I made: #28544 (unfinished, buggy). If this one could work then instead of failing directly it would at least retry later. Next steps:
cc @jvillafanez |
There is more research about it in https://github.com/owncloud/files_onedrive/issues/37#issuecomment-324074762 |
Thanks, this looks scary: a caldav plugin triggering opendir / scanning ?! In general I wondered whether we should simply disable any scanning when the operation is not PROPFIND, GET or MOVE. It doesn't make much sense to rescan for changes for a DELETE. Hmmm, well... maybe it does, because a DELETE moves stuff to trashbin and needs the filecache entries to be up to date before moving ? |
I think the most important thing is to know what is causing the lock of the parent folder, which, I guess, is what is preventing the rest of the deletions to be executed. Ideally, you should only lock the folder you're going to delete, so you shouldn't collide with the deletion of other sibling folders. Whether we should (deep) scan the deleted folder or not, that's a decision for the trashbin app, not for core, but it should only affect to the folder that's going to be deleted. This implies that deletion of sibling folders shouldn't collide with eachother |
Tested using redis (https://github.com/owncloud/files_onedrive/issues/37#issuecomment-324879207) Works fine, no errors. But another bug has appeared: |
Did you tested with several big folders? In this case the deletion should take some time since it needs to move the contents to the trashbin. If it works in this scenario with redis, it will likely be a problem in the DB implementation. |
I just tried with the RetryLockingProvider out of curiosity, but it doesn't solve the problem: #28544. Anyway, this PR isn't ready as it doesn't seem to retry properly. |
Oh, if I set "filelocking.retrydelay" to 5000 in #28544 then the deletion work. But I suspect that the retry delay might need to be high enough to cover for the rescan cost... Might be a quick general workaround but the real solution would be to prevent the rescan in the first place. |
Not happening using 10.0.2. |
Ah! Then there is hope! |
I'll bisect this |
I added some logging in the locking code to log what path is logged when, including stack traces, and this for both endpoints. There are bigger differences: https://cloud.owncloud.com/index.php/s/VK1ncpEUdvhScz9 Now need to take some time to analyze this... |
additional note: the above was produced with minimal Sabre plugins on new dav |
The first It seems it's due to the differences in I don't think we can use our custom ObjectTree like we did before, because now the node tree isn't only about files but also about collections of different types. So we either need to make sure that such |
Making FileInfo lazy for the tree traversal algo is not an option because we need to getFileInfo to find out if a node even exists. So far the only way I see is to bring back a custom ObjectTree implementation which itself will detect if we're querying a under "files/$userId". If yes, then shortcut a bit and go directly to the target node. |
|
Pfff, the reason why DAVACL is messing up too is because it's checking privileges on all parents recursively. So when we do an operation on "files/$userId/sftp/a" it will recurse through every parent and call We could prevent DAVACL to check every parent when dealing with the "files/*" subtree as a workaround... Seems these are many workarounds due to the fact that accessing every parent is expensive and messes up with locks. |
Another completely different solution that comes to mind would be to only allow rescanning of everything that is below the currently requested node. If one of the parent nodes is retrieved, don't rescan or check for changes. This would likely solve the locking issue but not the filecache performance issue as we'd still be querying the file cache for every parent due to Sabre node iteration. |
The subtle thing about parallel operations on an external storage with update detection enabled is that one of the operation operates on disk and the second operation might detect this change on-disk first before the first operation finishes. This is why we need to keep scanner locking anyway. |
First working fix, the one with the tree shortcut and disabling DAVACL: #28853 @jvillafanez @SergioBertolinSG this solves the problem but might not be a satisfactory solution. I'll have a look if an alternative solution like #28779 (comment) is possible... |
Worst case we revert web UI to use the old DAV endpoint. However this doesn't exclude issues with parallel deletion initiated by newer desktop clients which use the new DAV endpoint... |
POC of alternative solution: limits the watcher to any subpaths of the currently requested path: #28855. So whenever any of the parents gets queried by the Sabre tree, the watcher/scanner doesn't kick in. The part I dislike about this approach is that it doesn't solve the parent querying / filecache queries like #28853 does. |
Despite the fact that the desktop client 2.3.3 is using the new DAV endpoint and is doing the three DELETE operations in parallel, I didn't manage to run into the locking situation. Maybe the browser doesn't do it exactly in parallel but with a slight microscopic delay ? Anyway, this makes this bug less critical. I think we'll switch the web UI to old DAV in the meantime. Because the fix itself #28853 is complex and requires more discussions regarding the DAV architecture. |
This PR reverts the web UI back to the old DAV endpoint as a workaround for the next release: #28914 |
This works in current master, close? |
Ok, let's close. We'll need to redo the switch to new DAV, I'll put this on my PR #28853 |
actually I've put the switch in a separate PR here: #28874 which also contains switching for single file uploads |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Steps to reproduce
Expected behaviour
All folders are removed.
Actual behaviour
There are errors. The first one is erased, but the next are locked.
Server configuration
Operating system:
Ubuntu 16.04
Web server:
Apache
Database:
MySQL
PHP version:
7.0
ownCloud version: (see ownCloud admin page)
(current master not stable10)
{"installed":"true","maintenance":"false","needsDbUpgrade":"false","version":"10.0.3.0","versionstring":"10.0.3 beta","edition":"Community","productname":"ownCloud"}
Updated from an older ownCloud or fresh install:
Fresh
The content of config/config.php:
Are you using external storage, if yes which one: local/smb/sftp/...
Yes. SFTP
Are you using encryption:
No.
Logs
Client configuration
Browser
Chrome
Ref: https://github.com/owncloud/files_onedrive/issues/37
The text was updated successfully, but these errors were encountered: