cache loadUser if not exists #27068

Merged
merged 4 commits into from Feb 16, 2017

Conversation

@butonic
Member

butonic commented Feb 1, 2017

TL;dr

This PR caches a non existing user in the DB, killing ~30 queries on every request when an ldap user logs in.

Long version

user_ldap (type authentication) depends upon the AvatarManager, which in turn triggers a setupfs(), which in turn pulls in all apps of type 'filesystem' (activity, dav, files, files_sharing, files_trashbin and files_versions. Probably objectstore, search_elastic / search_lucene and whatever app has type filesystem).

Loading an app causes the I10n to be initialized whenever \OC::$server->getL10N() is used in the app.php directly or if it is a dependency of the other classes used when enabling the app.

L10N tries to get the user object which somewhere calls userExists() ... aaaand that is why for every request we do ~30 SELECT "uid", "displayname" FROM "oc_users" WHERE LOWER("uid") = LOWER(?) queries. when loading the user_ldap app only user_database is enabled and since a non existing user is not cached the userExists check is constantly walking to the db ...

There are probably other things doing a userExists check, but L10N is the most frequent.

@DeepDiver1975 I don't know if the user backend loading order may be the cause for the mixed UI languages issue you were investigating. It sure smells like it.

Yes, this only removes a symptom of the mess described above. Maybe this is no longer an issue whan the user table has been merged. I don't know. Looking at the app loading made my head hurt because we are constantly calling \OC_App::registerAutoloading() for apps that don't even match the type

// Add each apps' folder as allowed class path
foreach($apps as $app) {
$path = self::getAppPath($app);
if($path !== false) {
self::registerAutoloading($app, $path);
}
}
. And then we are calling it again when loading the app:
self::registerAutoloading($app, $appPath);
馃挬

cc @PhilippSchaffrath

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 1, 2017

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

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

@butonic butonic added this to the 9.1.5 milestone Feb 1, 2017

@butonic butonic requested review from DeepDiver1975 and phisch Feb 1, 2017

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Feb 1, 2017

Member

oh and because the user_ldap backend is not yet loaded adding this caching to the User Manager doesn't help. It's a mess ... @felixboehm

Member

butonic commented Feb 1, 2017

oh and because the user_ldap backend is not yet loaded adding this caching to the User Manager doesn't help. It's a mess ... @felixboehm

@butonic butonic added the blue-ticket label Feb 1, 2017

lib/private/User/Database.php
@@ -78,6 +78,7 @@ public function __construct() {
* itself, not in its subclasses.
*/
public function createUser($uid, $password) {
+ unset($this->cache[$uid]); // invalidate cache

This comment has been minimized.

@phisch

phisch Feb 1, 2017

Contributor

indentation + why do we need this? is it because a user could have been deteled and re-created while cache entries still exist?

@phisch

phisch Feb 1, 2017

Contributor

indentation + why do we need this? is it because a user could have been deteled and re-created while cache entries still exist?

lib/private/User/Database.php
@@ -206,6 +207,7 @@ public function checkPassword($uid, $password) {
if(!empty($newHash)) {
$this->setPassword($uid, $password);
}
+ unset($this->cache[$uid]); // invalidate cache

This comment has been minimized.

@phisch

phisch Feb 1, 2017

Contributor

is this just so a login returns fresh data?

@phisch

phisch Feb 1, 2017

Contributor

is this just so a login returns fresh data?

This comment has been minimized.

@butonic

butonic Feb 1, 2017

Member

before checkPassword the user might have been cachad as non existing. this clears the cache and allows subsequent calls to return the new user object.

@butonic

butonic Feb 1, 2017

Member

before checkPassword the user might have been cachad as non existing. this clears the cache and allows subsequent calls to return the new user object.

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

I'm still unsure with this change... what if a valid user checks its right password? it shouldn't be needed to reset the cache.
What about resetting if the password is wrong?

@jvillafanez

jvillafanez Feb 16, 2017

Member

I'm still unsure with this change... what if a valid user checks its right password? it shouldn't be needed to reset the cache.
What about resetting if the password is wrong?

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

I think this part of the function has nothing to do with checking the password.

In old versions of OC the hashing algo used for passwords was different and less secure.
Here if we found that the password matches, we take the opportunity to rehash the password with the new algo, since the plain text password is available. This is why setPassword is called.

@PVince81

PVince81 Feb 16, 2017

Member

I think this part of the function has nothing to do with checking the password.

In old versions of OC the hashing algo used for passwords was different and less secure.
Here if we found that the password matches, we take the opportunity to rehash the password with the new algo, since the plain text password is available. This is why setPassword is called.

lib/private/User/Database.php
- }
+ } else {
+ $this->cache[$uid] = false;
+ }

This comment has been minimized.

@phisch

phisch Feb 1, 2017

Contributor

indentation

@phisch

phisch Feb 1, 2017

Contributor

indentation

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Feb 1, 2017

Member
Test\User\DatabaseTest::testLogin
Failed asserting that false is true.

/var/lib/jenkins/workspace/owncloud-core_core_PR-27068-JVANWQJORMVZ2ZFTRBX7EDETA7IPFGHFJDKLNVWCO3LGWDFOURRA/tests/lib/User/BackendTestCase.php:84
Member

DeepDiver1975 commented Feb 1, 2017

Test\User\DatabaseTest::testLogin
Failed asserting that false is true.

/var/lib/jenkins/workspace/owncloud-core_core_PR-27068-JVANWQJORMVZ2ZFTRBX7EDETA7IPFGHFJDKLNVWCO3LGWDFOURRA/tests/lib/User/BackendTestCase.php:84
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 7, 2017

Member

Ok now I understand this PR. I think it makes sense.

Member

PVince81 commented Feb 7, 2017

Ok now I understand this PR. I think it makes sense.

@PVince81 PVince81 added the sev2-high label Feb 8, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 8, 2017

Member

I'm wondering about other code paths affected by this cache value.

For example when deleting a DB user, the cache value should also be reset. Or it should be set to false directly.

Member

PVince81 commented Feb 8, 2017

I'm wondering about other code paths affected by this cache value.

For example when deleting a DB user, the cache value should also be reset. Or it should be set to false directly.

@butonic

This comment has been minimized.

Show comment
Hide comment
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 10, 2017

Member

Ah yes indeed. It wasn't in your changeset but old code.

Member

PVince81 commented Feb 10, 2017

Ah yes indeed. It wasn't in your changeset but old code.

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Feb 15, 2017

Contributor

(I always wondered why oC does not cache more stuff in the PHP session. Is it because the PHP session could be much more long lived?)

Contributor

guruz commented Feb 15, 2017

(I always wondered why oC does not cache more stuff in the PHP session. Is it because the PHP session could be much more long lived?)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 15, 2017

Member

I'm fine with the fix then.

Would be good to have a unit test to check for the proper cache clearing.

@butonic should someone take over ?

Member

PVince81 commented Feb 15, 2017

I'm fine with the fix then.

Would be good to have a unit test to check for the proper cache clearing.

@butonic should someone take over ?

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic Feb 15, 2017

Member

@PVince81 that would bring tears of joy to my eyes ...

Member

butonic commented Feb 15, 2017

@PVince81 that would bring tears of joy to my eyes ...

@PVince81 PVince81 self-assigned this Feb 15, 2017

lib/private/User/Database.php
@@ -220,7 +223,7 @@ public function checkPassword($uid, $password) {
* @return boolean
*/
private function loadUser($uid) {
- if (empty($this->cache[$uid])) {
+ if (!isset($this->cache[$uid]) && $this->cache[$uid] !== false) {

This comment has been minimized.

@PVince81

PVince81 Feb 15, 2017

Member

I'll change this to array_key_exists here for better clarity.

Here we only want to check if the key exists even though it can have the value false. But isset would return false even if the key exists but its value is falsy.
array_key_exists will do the check we are actually expecting without confusion.

@PVince81

PVince81 Feb 15, 2017

Member

I'll change this to array_key_exists here for better clarity.

Here we only want to check if the key exists even though it can have the value false. But isset would return false even if the key exists but its value is falsy.
array_key_exists will do the check we are actually expecting without confusion.

This comment has been minimized.

@PVince81

PVince81 Feb 15, 2017

Member

Uh oh... array_key_exists doesn't work here. But it works in a separate script.

Maybe 'CappedMemoryCache` is not overriding shit properly...

I sense recursive bug finding mode approaching...

@PVince81

PVince81 Feb 15, 2017

Member

Uh oh... array_key_exists doesn't work here. But it works in a separate script.

Maybe 'CappedMemoryCache` is not overriding shit properly...

I sense recursive bug finding mode approaching...

This comment has been minimized.

@PVince81

PVince81 Feb 15, 2017

Member

Okay, two unhappy findings...

  1. CappedMemoryCache (which implements ArrayAccess) uses isset too for key existence: https://github.com/owncloud/core/blob/master/lib/private/Cache/CappedMemoryCache.php#L41.
    Since we're anyway using isset almost everywhere this shouldn't be a problem, hopefully...

  2. When debugging, array_key_exists doesn't seem to call any of the methods of the capped cache, so probably not even compatible with ArrayAccess. And that was tested on PHP 7.0 ...

Ok, enough diverging... I'll set this back as I found it.

@PVince81

PVince81 Feb 15, 2017

Member

Okay, two unhappy findings...

  1. CappedMemoryCache (which implements ArrayAccess) uses isset too for key existence: https://github.com/owncloud/core/blob/master/lib/private/Cache/CappedMemoryCache.php#L41.
    Since we're anyway using isset almost everywhere this shouldn't be a problem, hopefully...

  2. When debugging, array_key_exists doesn't seem to call any of the methods of the capped cache, so probably not even compatible with ArrayAccess. And that was tested on PHP 7.0 ...

Ok, enough diverging... I'll set this back as I found it.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 15, 2017

Member

@PVince81 that would bring tears of joy to my eyes ...

@butonic take this napkin.

I've added unit tests. If you comment out some of the unset they will fail.

@jvillafanez @IljaN @DeepDiver1975 review ?

Member

PVince81 commented Feb 15, 2017

@PVince81 that would bring tears of joy to my eyes ...

@butonic take this napkin.

I've added unit tests. If you comment out some of the unset they will fail.

@jvillafanez @IljaN @DeepDiver1975 review ?

lib/private/User/Database.php
if (!$this->userExists($uid)) {
+ unset($this->cache[$uid]); // invalidate non existing user in cache

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

This seems unneeded if the first thing we do is unsetting the same uid.

@jvillafanez

jvillafanez Feb 16, 2017

Member

This seems unneeded if the first thing we do is unsetting the same uid.

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

userExists will see that the user does not exist and it will cache false for that user, so we need to reset it here.

If you comment out that line some of the new tests will fail.

@PVince81

PVince81 Feb 16, 2017

Member

userExists will see that the user does not exist and it will cache false for that user, so we need to reset it here.

If you comment out that line some of the new tests will fail.

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Maybe it's clearer if we unset the user if it's created correctly in the DB.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Maybe it's clearer if we unset the user if it's created correctly in the DB.

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

done

lib/private/User/Database.php
@@ -206,6 +207,7 @@ public function checkPassword($uid, $password) {
if(!empty($newHash)) {
$this->setPassword($uid, $password);
}
+ unset($this->cache[$uid]); // invalidate cache

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

I'm still unsure with this change... what if a valid user checks its right password? it shouldn't be needed to reset the cache.
What about resetting if the password is wrong?

@jvillafanez

jvillafanez Feb 16, 2017

Member

I'm still unsure with this change... what if a valid user checks its right password? it shouldn't be needed to reset the cache.
What about resetting if the password is wrong?

lib/private/User/Database.php
$this->cache[$uid]['uid'] = $row['uid'];
$this->cache[$uid]['displayname'] = $row['displayname'];
+ } else {
+ $this->cache[$uid] = false;

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

Indentation here

@jvillafanez

jvillafanez Feb 16, 2017

Member

Indentation here

lib/private/User/Database.php
@@ -229,9 +233,12 @@ private function loadUser($uid) {
return false;
}
- while ($row = $result->fetchRow()) {
+ if ($row = $result->fetchRow()) {

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

If we're relying that the query result will be unique, I'd add a comment here.

@jvillafanez

jvillafanez Feb 16, 2017

Member

If we're relying that the query result will be unique, I'd add a comment here.

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

uid is the primary key of the table so it can only be unique. (I also wondered why while was used before, probably a copy-paste mistake)

@PVince81

PVince81 Feb 16, 2017

Member

uid is the primary key of the table so it can only be unique. (I also wondered why while was used before, probably a copy-paste mistake)

@@ -205,6 +207,7 @@ public function checkPassword($uid, $password) {
if(\OC::$server->getHasher()->verify($password, $storedHash, $newHash)) {
if(!empty($newHash)) {
$this->setPassword($uid, $password);
+ unset($this->cache[$uid]); // invalidate cache

This comment has been minimized.

@PVince81

PVince81 Feb 16, 2017

Member

@jvillafanez I moved this here now to make it cleared that it's setPassword that might indirectly cache the user through its userExists call

@PVince81

PVince81 Feb 16, 2017

Member

@jvillafanez I moved this here now to make it cleared that it's setPassword that might indirectly cache the user through its userExists call

This comment has been minimized.

@jvillafanez

jvillafanez Feb 16, 2017

Member

looks better now 馃憤

@jvillafanez

jvillafanez Feb 16, 2017

Member

looks better now 馃憤

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 16, 2017

Member

@jvillafanez 414d3d5

I also added closeCursor just to be safe

Member

PVince81 commented Feb 16, 2017

@jvillafanez 414d3d5

I also added closeCursor just to be safe

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Feb 16, 2017

Member

馃憤 as long as the tests pass

Member

jvillafanez commented Feb 16, 2017

馃憤 as long as the tests pass

@PVince81 PVince81 merged commit 42ed0ab into master Feb 16, 2017

4 checks passed

Scrutinizer 2 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the cache-load-user-if-not-exists branch Feb 16, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 16, 2017

Member

@butonic backport down to 9.1 only or further ?

Member

PVince81 commented Feb 16, 2017

@butonic backport down to 9.1 only or further ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 16, 2017

Member

stable9.1: #27174

Member

PVince81 commented Feb 16, 2017

stable9.1: #27174

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 10, 2017

Member

stable9: #27351

Member

PVince81 commented Mar 10, 2017

stable9: #27351

@MorrisJobke MorrisJobke referenced this pull request in nextcloud/server Mar 20, 2017

Merged

cache loadUser if not exists #3929

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