Fix DAV stat cache to properly cache 404 #26324

Merged
merged 1 commit into from Nov 8, 2016

Projects

None yet

4 participants

@PVince81
Collaborator

Description

404 errors were not properly cached due to catching the wrong
exception. Now catching ClientHttpException and checking the error
code. In case of 404, adjust the stat cache accordingly.

Since now the local propfind() call throws NotFound, there is no need
to catch ClientHttpException everywhere any more but need to catch
NotFound exception instead and act accordingly.

Related Issue

Fixes #26323

Motivation and Context

Improve performance when creating new file/folders which usually first do a PROPFIND on the remote server to check for file/folder existence. Not caching these 404 would cause at least 6 additional PROPFIND requests to get the status. (this is because we use a lot of file_exists call everywhere...)

How Has This Been Tested?

See issue steps.

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.

Backports

  • TODO: evaluate to which versions to backport this

@DeepDiver1975 @butonic @jvillafanez

@PVince81 PVince81 added this to the 9.2 milestone Oct 10, 2016
@mention-bot

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @icewind1991 and @LukasReschke to be potential reviewers.

@PVince81
Collaborator

also:

  • do proper regression testing, I'm not sure this is really covered by CI ...
@PVince81
Collaborator

Okay, the storage impl is covered by the DAV ext storage tests at least. The bug itself wouldn't trigger any failure since it was a caching issue.

@DeepDiver1975
Member
21:49:28 There was 1 failure:
21:49:28 
21:49:28 1) OCA\Files_Sharing\Tests\CacheTest::testGetFolderContentsInRoot
21:49:28 Failed asserting that 10 matches expected 3.
21:49:28 
21:49:28 /var/lib/jenkins/workspace/owncloud-core_core_PR-26324-CAYE3ELZQFIMHVKICLMMAFO6AP4X6ZQLHJBL3TNLCB4KDPW5UN2A/apps/files_sharing/tests/CacheTest.php:444
21:49:28 /var/lib/jenkins/workspace/owncloud-core_core_PR-26324-CAYE3ELZQFIMHVKICLMMAFO6AP4X6ZQLHJBL3TNLCB4KDPW5UN2A/apps/files_sharing/tests/CacheTest.php:349
- $this->statCache->clear($path . '/');
- $this->statCache->set($path, false);
- throw $e;
+ } catch (ClientHttpException $e) {
@jvillafanez
jvillafanez Oct 18, 2016 Contributor

Is this change intentional? I find strange that we're catching "NotFound" exceptions and removing "ClientHttpException" ones except here ๐Ÿ˜•

@PVince81
PVince81 Oct 18, 2016 Collaborator

It's a bit messy...

Basically Sabre's propfind itself only ever throws ClientHttpException or ClientException, never NotFound (that one belongs to sabre server, not sabre client btw).

But here we do want to throw a known exception for the consumers of our own $this->propfind, so we throw NotFound and they all catch it.

Maybe it would be cleaner to simply rethrow ClientHttpException as is to avoid confusion ?

@jvillafanez
jvillafanez Oct 18, 2016 Contributor

I'd rather keep consistency on the client side: if client expects a "NotFound" exception then we throw that exception.

@PVince81
PVince81 Oct 18, 2016 Collaborator

The client doesn't expected NotFound exception, only the consumers of $this->propFind do, and they're all within this single class.

@PVince81
PVince81 Oct 18, 2016 Collaborator

Argh, looks like I can't easily replace with only ClientHttpException, because when a 404 propfind was cached, I'd need to create a virtual ClientHttpException to simulate failure here: https://github.com/owncloud/core/pull/26324/files#diff-eaf59cbf225a9e583d7cda98942ed6abR253

That's the reason why I picked this exception. Not sure if there is a better solution.

@jvillafanez
jvillafanez Oct 19, 2016 Contributor

If there is no easy solution, let's go with this. Just write a comment to remind why we've chosen this decision.

@PVince81
PVince81 Oct 19, 2016 Collaborator

Sounds good, will do

@PVince81
PVince81 Oct 20, 2016 Collaborator

Now the tricky part: create a virtual ClientHttpException considering that it takes a ResponseInterface in its constructor... I considered either creating a class that extends ClientHttpException or one that implements ResponseInterface but it all goes too far, too many methods.

I'll see if simply returning false from propfind and handling this outside would be better. (outside being inside the class as propfind is private)

@PVince81
Collaborator

Oh, PHP 7.1 specific failure... fancy!

@PVince81
Collaborator

hmm, tests pass for me in PHP 7.0.12... will need to setup PHP 7.1 then

@PVince81
Collaborator

I've setup 7.1.0RC4 with phpenv (yay, it worked!) but the tests do pass.
On Jenkins it's 7.1.0RC1, not sure if related.
Let's restart the build and see...

If it fails again, then we can try upgrading Jenkins to 7.1.0RC4.
And if it still fails, then it's possibly due to test order...

@PVince81
Collaborator

This is the failure on Jenkins:

15:10:51 1) OCA\Files_Sharing\Tests\CacheTest::testGetFolderContentsInRoot
15:10:51 Failed asserting that 10 matches expected 3.
15:10:51 
15:10:51 /var/lib/jenkins/workspace/owncloud-core_core_PR-26324-CAYE3ELZQFIMHVKICLMMAFO6AP4X6ZQLHJBL3TNLCB4KDPW5UN2A/apps/files_sharing/tests/CacheTest.php:444
15:10:51 /var/lib/jenkins/workspace/owncloud-core_core_PR-26324-CAYE3ELZQFIMHVKICLMMAFO6AP4X6ZQLHJBL3TNLCB4KDPW5UN2A/apps/files_sharing/tests/CacheTest.php:349
15:10:51 
@PVince81
Collaborator

Rebased, let's hope the test magically passes now...

@PVince81
Collaborator

Even with PHP 7.1.0RC1 the problem doesn't appear on my local env.

So it seems that all I can do now is guessing and trial and error.

@PVince81
Collaborator

The failure itself seems completely unrelated to this change.
I could imagine that the failure is rather related to another PR that was merged into master related to etag propagation. But then it should all also fail on master then.

@PVince81
Collaborator

Jenkins PR: #26466

Let's see if it also fails if restarted in that separate test env...

@PVince81
Collaborator

Ha, the Jenkins PR healed it...

@PVince81
Collaborator

@jvillafanez I decided to use the return false approach for the private propfind method when an entry was not found. It seems to work fine.

Please rereview.

@PVince81
Collaborator

@jvillafanez please re-review

lib/private/Files/Storage/DAV.php
@@ -248,15 +242,11 @@ public function opendir($path) {
*
* @return array propfind response
*
- * @throws NotFound
+ * @throws ClientHttpException
@jvillafanez
jvillafanez Nov 7, 2016 Contributor

This doc should be updated with the conditions under it returns "false"

@jvillafanez
Contributor

I don't see anything strange so ๐Ÿ‘

@PVince81
Collaborator
PVince81 commented Nov 7, 2016

Adjusted comment, squashed and rebased.

@PVince81
Collaborator
PVince81 commented Nov 7, 2016

Grrrr, that unrelated failure again:

10:13:15 1) OCA\Files_Sharing\Tests\CacheTest::testGetFolderContentsInRoot
10:13:15 Failed asserting that 10 matches expected 3.
10:13:15 
10:13:15 /var/lib/jenkins/workspace/owncloud-core_core_PR-26324-CAYE3ELZQFIMHVKICLMMAFO6AP4X6ZQLHJBL3TNLCB4KDPW5UN2A/apps/files_sharing/tests/CacheTest.php:444
10:13:15 /var/lib/jenkins/workspace/owncloud-core_core_PR-26324-CAYE3ELZQFIMHVKICLMMAFO6AP4X6ZQLHJBL3TNLCB4KDPW5UN2A/apps/files_sharing/tests/CacheTest.php:349
10:13:15 

Jenkins PR for the rescue: #26564

@PVince81 PVince81 Fix DAV stat cache to properly cache 404
404 errors were not properly cached due to catching the wrong
exception. Now catching ClientHttpException and checking the error
code. In case of 404, adjust the stat cache accordingly.
48093ef
@PVince81
Collaborator
PVince81 commented Nov 8, 2016

Tests passed here #26574 and travis failure unrelated.

@PVince81 PVince81 merged commit 0a6fc26 into master Nov 8, 2016

3 of 4 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
Scrutinizer 2 new issues, 6 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the fed-statcache-404 branch Nov 8, 2016
@PVince81
Collaborator
PVince81 commented Dec 7, 2016

stable9.1: #26791

@PVince81 PVince81 referenced this pull request Dec 7, 2016
Merged

[stable9] Fix DAV stat cache to properly cache 404 #26792

1 of 1 task complete
@PVince81
Collaborator
PVince81 commented Dec 7, 2016

stable9: #26792

@rullzer rullzer referenced this pull request in nextcloud/server Dec 19, 2016
Merged

[downstream] Fix DAV stat cache to properly cache 404 #2744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment