Don't query Sabre parent nodes by PVince81 · Pull Request #28853 · owncloud/core

PVince81 · 2017-08-30T13:04:49Z

Description

When dealing with file access, the old DAV had a shortcut way to
directly access a file node without having to traverse all parents.
The new DAV endpoint needs this as well:

added \OCA\DAV\Tree implementation with shortcut for "files"
disable DAVACL plugin as it would also query all parents

There are several reasons for avoiding to query the parents:

it causes file cache access for each node, not removable as it needs
to check for existence
with external storages and update detection, querying a folder node
causes update detection to kick in and the scanner to do its work, the
scanner will set additional locks that might needlessly prevent
concurrent access.

The sad part is that any future Sabre plugin without caring for the above has a risk of retriggering these issues. So any Sabre plugin dealing with files must be aware of this and avoid needlessly querying every parent node.

Related Issue

Fixes #28779

Motivation and Context

Read from #28779 (comment).
But also this will improve performance or at least bring it closer to the one from the old DAV endpoint by avoiding additional file cache queries.

How Has This Been Tested?

Test steps from above ticket: deletion of three folders simultaneously on an external storage with update detection kept enabled.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

jvillafanez · 2017-08-30T16:55:16Z

with external storages and update detection, querying a folder node
causes update detection to kick in and the scanner to do its work, the
scanner will set additional locks that might needlessly prevent
concurrent access.

This should be fine unless you're deleting a parent (at any level) of another folder that is being deleted at the same. In this case it's fine because there are operations that are happening in the contents of the folder.

I think just disabling the ACL plugin should be enough to solve the problem.

PVince81 · 2017-08-30T17:01:14Z

I think just disabling the ACL plugin should be enough to solve the problem.

It's not. I already tried: both fixes are needed else the 423 Locked exceptions will creep in.
This is because the object Tree also touches parent nodes

jvillafanez · 2017-08-30T17:19:05Z

+		}
+
+		// check the path, also called when the path has been entered manually eg via a file explorer
+		if (\OC\Files\Filesystem::isForbiddenFileOrDir($path)) {


any reason for this duplication? it will a comment if any.

jvillafanez · 2017-08-30T17:21:00Z

+		}
+
+		// Is it the root node?
+		if ($path === '') {


move this check up, before any complex operation?

jvillafanez · 2017-08-30T17:22:09Z

+			throw new \Sabre\DAV\Exception\Forbidden();
+		}
+
+		$path = trim($path, '/');


we could move this before the isForbiddenFileOrDir check

hah... seems it's also duplicated in ObjectTree...

jvillafanez · 2017-08-30T17:24:02Z

+
+		$path = trim($path, '/');
+
+		if (isset($this->cache[$path]) && $this->cache[$path] !== false) {


this also might be possible to move it before the isForbiddenFileOrDir check, to return the cache data as soon as we can

jvillafanez · 2017-08-30T17:29:47Z

+
+		if (!$info) {
+			$this->cache[$path] = false;
+			throw new \Sabre\DAV\Exception\NotFound('File with name ' . $path . ' could not be located');


Do we want to cache this exception? I mean, if the file doesn't exists at some point, it won't likely exists during the rest of the request, so we might not want to go through all the steps again to get another NotFound. On the other hand, it might be fine since it's expected the exception isn't catched until the top level to send the message to the client.

the exception is cached already with the value false for future checks

PVince81 · 2017-08-30T17:35:05Z

@jvillafanez I actually copied getNodeForPath from the old ObjectTree::getNodeForPath and removed every bit related to chunking or part files. Then I added a few lines at the top to compute the correct path. The result is what you see above.

I'll still have a look at your proposals for optimization, might be a good opportunity.

Before I do, do you agree with the general approach ?

I'm not sure if @DeepDiver1975 would appreciate that we brought back the Tree implementation but so far I don't see any better way. Too bad Sabre doesn't have a "getNodeForPath" event that plugins could override.

jvillafanez · 2017-08-31T06:52:54Z

The only option left that I see in addition to this one is to override the RootCollection instead of the \OCA\DAV\Tree. I don't know what is the best option.

The other option would be to modify the View and change or add a method to get the file info without triggering a scan, but in addition to be risky (too many components might be affected) we'll need to evaluate what could be the consecuences of fetching not up-to-date data.

PVince81 · 2017-08-31T08:15:57Z

The only option left that I see in addition to this one is to override the RootCollection instead of the \OCA\DAV\Tree. I don't know what is the best option.

We already have our own RootCollection. Sadly the API getChild() only allows to get the next child, not a sub-sub-sub-child. And the calling code also calls it this way, recursively, step by step.

The other option would be to modify the View and change or add a method to get the file info without triggering a scan, but in addition to be risky (too many components might be affected) we'll need to evaluate what could be the consecuences of fetching not up-to-date data.

I looked into this here: #28855. You're right that it's risky so I'm less happy with #28855 than with #28853

PVince81 · 2017-08-31T09:06:21Z

adjust getNodeForPath based on comments
fix unit tests
write new unit tests for the new Tree class

PVince81 · 2017-08-31T09:07:52Z

double check that cached false will rethrow exception if queried again => won't do for now, I want to keep the old behavior as close as possible to the one of ObjectTree. To be fixed separately

PVince81 · 2017-09-01T14:47:40Z

In theory we could invent a new interface that allows our sabre collections to provide "getNodeForPath" for a subpath. If implemented, call it. Else fall back to traversing with getChild()

PVince81 · 2017-09-01T15:04:50Z

double check $this->tree->cacheNode() usage in Directory, might not be using the correct path...

PVince81 · 2017-09-01T15:16:40Z

We might not even need cacheNode() any more thanks to: https://github.com/fruux/sabre-dav/blob/master/lib/DAV/Tree.php#L200.

But I also found this... #21401

PVince81 · 2017-09-01T15:19:43Z

Now from reading the comments, it seems that doing the cacheNode thing was relevant mostly for the old DAV endpoint where we had the custom ObjectTree. Now in the new DAV endpoint there is no such tree. Well, there will be one added again from this PR, so not sure...

I think re-adding getNodeForPath is likely the correct way after reading this comment: #21401 (comment)

PVince81 · 2017-09-01T15:20:50Z

I have the feeling that this is going a bit too far already with risk of regressions or at least performance regressions... If I don't find a solution until next week I'll revert the web UI to use the old DAV endpoint for the time being.

SergioBertolinSG · 2017-09-04T08:24:47Z

After mounting an external storage the web ui has a spinner spinning forever, refresh doesn't help.

logs:

{"reqId":"PSivBUEMCZ1i4bR7V8ea","level":3,"time":"2017-09-04T08:25:55+00:00","remoteAddr":"hidden_ip","user":"admin","app":"remote","method":"PROPFIND","url":"\/remote.php\/dav\/files\/admin\/","message":"Exception: {\"Exception\":\"TypeError\",\"Message\":\"Argument 2 passed to Sabre\\\\DAV\\\\Server::getPropertiesByNode() must implement interface Sabre\\\\DAV\\\\INode, null given, called in \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php on line 980\",\"Code\":0,\"Trace\":\"#0 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(980): Sabre\\\\DAV\\\\Server->getPropertiesByNode(Object(Sabre\\\\DAV\\\\PropFind), NULL)\\n#1 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(1665): Sabre\\\\DAV\\\\Server->getPropertiesIteratorForPath('files\\\/admin', Array, 1)\\n#2 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(355): Sabre\\\\DAV\\\\Server->generateMultiStatus(Object(Generator), false)\\n#3 [internal function]: Sabre\\\\DAV\\\\CorePlugin->httpPropFind(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#4 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\\n#5 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(479): Sabre\\\\Event\\\\EventEmitter->emit('method:PROPFIND', Array)\\n#6 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(254): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#7 \\\/opt\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Server.php(243): Sabre\\\\DAV\\\\Server->exec()\\n#8 \\\/opt\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(31): OCA\\\\DAV\\\\Server->exec()\\n#9 \\\/opt\\\/owncloud\\\/remote.php(175): require_once('\\\/opt\\\/owncloud\\\/a...')\\n#10 {main}\",\"File\":\"\\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php\",\"Line\":1056}"}

SergioBertolinSG · 2017-09-04T08:48:05Z

Retested, external storage is not required, it fails just opening files view 👎 :

{"reqId":"ZgobgAAahizRXXgw15PQ","level":3,"time":"2017-09-04T08:45:32+00:00","remoteAddr":"::1","user":"--","app":"PHP","method":"GET","url":"\/index.php","message":"Undefined variable: user at \/opt\/owncloud\/lib\/public\/Util.php#193"}
{"reqId":"yX6y7isiGhJ5tfbBkRDp","level":3,"time":"2017-09-04T08:46:16+00:00","remoteAddr":"HIDDEN_IP","user":"admin","app":"remote","method":"PROPFIND","url":"\/remote.php\/dav\/files\/admin\/","message":"Exception: {\"Exception\":\"TypeError\",\"Message\":\"Argument 2 passed to Sabre\\\\DAV\\\\Server::getPropertiesByNode() must implement interface Sabre\\\\DAV\\\\INode, null given, called in \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php on line 980\",\"Code\":0,\"Trace\":\"#0 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(980): Sabre\\\\DAV\\\\Server->getPropertiesByNode(Object(Sabre\\\\DAV\\\\PropFind), NULL)\\n#1 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(1665): Sabre\\\\DAV\\\\Server->getPropertiesIteratorForPath('files\\\/admin', Array, 1)\\n#2 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(355): Sabre\\\\DAV\\\\Server->generateMultiStatus(Object(Generator), false)\\n#3 [internal function]: Sabre\\\\DAV\\\\CorePlugin->httpPropFind(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#4 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\\n#5 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(479): Sabre\\\\Event\\\\EventEmitter->emit('method:PROPFIND', Array)\\n#6 \\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(254): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#7 \\\/opt\\\/owncloud\\\/apps\\\/dav\\\/lib\\\/Server.php(243): Sabre\\\\DAV\\\\Server->exec()\\n#8 \\\/opt\\\/owncloud\\\/apps\\\/dav\\\/appinfo\\\/v2\\\/remote.php(31): OCA\\\\DAV\\\\Server->exec()\\n#9 \\\/opt\\\/owncloud\\\/remote.php(175): require_once('\\\/opt\\\/owncloud\\\/a...')\\n#10 {main}\",\"File\":\"\\\/opt\\\/owncloud\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php\",\"Line\":1056}"}

PVince81 · 2017-09-04T09:17:55Z

argh... I was doing something experimental and moved around code but made the mistake to commit the removal of said code

PVince81 · 2017-09-04T09:26:16Z

cache the full DAV path in Tree instead of just the files subpath... => I used a separate cache instead, because cachePath cannot reliably guess what user we're accessing (I didn't want to cheat with user session)

PVince81 · 2017-09-04T09:41:17Z

@SergioBertolinSG please try again, thanks

PVince81 · 2017-09-04T09:43:44Z

@jvillafanez mind rechecking ?

In general I'm not too happy about this but it feels like it's the best compromise so far.

I hope we can eventually get rid of cachePath which is called by `Directory::getChildren()' for every child. I believe that Sabre is supposed to have a way already to obsolete this, but the discussion from the ticket in #21401 (comment) makes me doubt about it and I don't want to change too much at once to avoid regressions. We really need to clean up this DAV mess at some point...

PVince81 · 2017-12-12T09:54:30Z

Transferred to @DeepDiver1975 for hijacking

codecov · 2017-12-12T11:08:48Z

Codecov Report

Merging #28853 into master will increase coverage by 0.03%.
The diff coverage is 83.01%.

@@             Coverage Diff              @@
##             master   #28853      +/-   ##
============================================
+ Coverage      62.6%   62.64%   +0.03%     
- Complexity    17584    17598      +14     
============================================
  Files          1038     1039       +1     
  Lines         57920    57956      +36     
============================================
+ Hits          36263    36305      +42     
+ Misses        21657    21651       -6

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Files/RootCollection.php	`15.38% <0%> (-9.62%)`	`4 <0> (ø)`
apps/dav/lib/Connector/Sabre/Directory.php	`70.25% <100%> (ø)`	`72 <1> (ø)`	⬇️
apps/dav/lib/Files/FilesHome.php	`100% <100%> (+100%)`	`12 <9> (+8)`	⬆️
apps/dav/lib/Server.php	`49.18% <100%> (+0.84%)`	`12 <0> (+1)`	⬆️
apps/dav/lib/Connector/Sabre/ObjectTree.php	`60.22% <66.66%> (+0.22%)`	`37 <0> (-1)`	⬇️
apps/dav/lib/Tree.php	`88.23% <88.23%> (ø)`	`6 <6> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2303c4...483d060. Read the comment docs.

PVince81 · 2017-12-13T11:24:18Z

👍 for @DeepDiver1975's changes

PVince81 · 2017-12-13T11:24:36Z

I see some tab issues, fixing... (copy-pasted code from Sabre that has spaces instead of tabs)

When dealing with file access, the old DAV had a shortcut way to directly access a file node without having to traverse all parents. The new DAV endpoint needs this as well: - added \OCA\DAV\Tree implementation with shortcut for "files" - disable DAVACL plugin as it would also query all parents There are several reasons for avoiding to query the parents: - it causes file cache access for each node, not removable as it needs to check for existence - with external storages and update detection, querying a folder node causes update detection to kick in and the scanner to do its work, the scanner will set additional locks that might needlessly prevent concurrent access.

PVince81 · 2017-12-13T11:31:21Z

I've squashed the tab fixes into the second commit and rebased onto master.

If CI passes let's merge this and backport.

lock · 2019-08-01T17:02:51Z

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.

PVince81 added the 3 - To Review label Aug 30, 2017

PVince81 added this to the development milestone Aug 30, 2017

PVince81 requested review from DeepDiver1975, butonic and jvillafanez August 30, 2017 13:04

PVince81 mentioned this pull request Aug 30, 2017

Deletion of several folders or files fails when using external storages. #28779

Closed

PVince81 added p1-urgent Critical issue, need to consider hotfix with just that issue backport-request labels Aug 30, 2017

PVince81 mentioned this pull request Aug 30, 2017

[WIP] Don't watch storage parents of current request #28855

Closed

9 tasks

PVince81 self-assigned this Aug 30, 2017

jvillafanez reviewed Aug 30, 2017

View reviewed changes

PVince81 force-pushed the newdav-leave-file-parents-alone branch from b481a9f to 03329e2 Compare September 4, 2017 09:40

PVince81 added this to the development milestone Nov 22, 2017

ownclouders added the status/STALE label Dec 7, 2017

DeepDiver1975 force-pushed the newdav-leave-file-parents-alone branch from 54ad05d to 451c835 Compare December 12, 2017 09:13

PVince81 assigned DeepDiver1975 Dec 12, 2017

DeepDiver1975 force-pushed the newdav-leave-file-parents-alone branch from 451c835 to cccee14 Compare December 12, 2017 11:08

DeepDiver1975 force-pushed the newdav-leave-file-parents-alone branch from cccee14 to a1298bc Compare December 12, 2017 13:03

ownclouders removed the status/STALE label Dec 12, 2017

DeepDiver1975 force-pushed the newdav-leave-file-parents-alone branch from a1298bc to 43ab959 Compare December 12, 2017 16:36

DeepDiver1975 approved these changes Dec 13, 2017

View reviewed changes

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Dec 13, 2017

Vincent Petry and others added 3 commits December 13, 2017 12:30

DAV FilesHome is now also a tree

6779ba2

Add more tests

483d060

PVince81 force-pushed the newdav-leave-file-parents-alone branch from 43ab959 to 483d060 Compare December 13, 2017 11:31

PVince81 added 4 - To release and removed 3 - To Review labels Dec 13, 2017

DeepDiver1975 merged commit b1df409 into master Dec 13, 2017

DeepDiver1975 deleted the newdav-leave-file-parents-alone branch December 13, 2017 14:45

DeepDiver1975 mentioned this pull request Dec 13, 2017

[stable10] Don't query Sabre parent nodes #29834

Merged

DeepDiver1975 removed the backport-request label Dec 13, 2017

ownclouders added the status/STALE label Dec 21, 2017

ownclouders removed the status/STALE label Jan 30, 2018

lock Bot locked as resolved and limited conversation to collaborators Aug 1, 2019


		$path = trim($path, '/');

		if (isset($this->cache[$path]) && $this->cache[$path] !== false) {

Uh oh!

Conversation

PVince81 commented Aug 30, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

jvillafanez commented Aug 30, 2017

Uh oh!

PVince81 commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Aug 30, 2017

Uh oh!

jvillafanez commented Aug 31, 2017

Uh oh!

PVince81 commented Aug 31, 2017

Uh oh!

PVince81 commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Sep 1, 2017

Uh oh!

PVince81 commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Sep 1, 2017

Uh oh!

PVince81 commented Sep 1, 2017

Uh oh!

PVince81 commented Sep 1, 2017

Uh oh!

SergioBertolinSG commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioBertolinSG commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Sep 4, 2017

Uh oh!

PVince81 commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Sep 4, 2017

Uh oh!

PVince81 commented Sep 4, 2017

Uh oh!

PVince81 commented Dec 12, 2017

Uh oh!

codecov Bot commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PVince81 commented Dec 13, 2017

Uh oh!

PVince81 commented Dec 13, 2017

Uh oh!

PVince81 commented Dec 13, 2017

PVince81 commented Aug 31, 2017 •

edited

Loading

PVince81 commented Aug 31, 2017 •

edited

Loading

PVince81 commented Sep 1, 2017 •

edited

Loading

SergioBertolinSG commented Sep 4, 2017 •

edited

Loading

SergioBertolinSG commented Sep 4, 2017 •

edited

Loading

PVince81 commented Sep 4, 2017 •

edited

Loading

codecov Bot commented Dec 12, 2017 •

edited

Loading