[stable9.1] Fix sharedstorage recursion hell #25789
Conversation
To prevent recursions in initMountPoints which requires the numeric id to populate oc_mounts
PVince81
force-pushed
the
stable9.1-usermountcache-hell-shortcutstorageid
branch
from
a045392 to
837bf1c
Compare
|
Rebased for CI. Please review @owncloud/sharing @owncloud/filesystem @DeepDiver1975 @VicDeo @jvillafanez |
PVince81
changed the title
| @@ -1684,6 +1686,11 @@ public function getPath($id) { | |||
| /** | |||
| * @var \OC\Files\Mount\MountPoint $mount | |||
| */ | |||
| if (!$includeShares && $mount instanceof SharedMount) { | |||
There was a problem hiding this comment.
Under what circumstances do we want to include the shares? Maybe we should specify when we can include the shares to make things clear, taking into account there is an infinite loop lurking. Otherwise, we should remove the flag and never instantiate shared storages.
There was a problem hiding this comment.
There are probably a few use cases I don't know of which would expect shares to be available.
One for example is the permalink: if you open a permalink where the fileid is on a shared storage, then the getPath method needs to be able to resolve it to the original storage by this mean.
Basically, I have the feeling that the only moment we need the shared mounts to be present here is for the current user, but not when recursing.
I chose the current somewhat hacking approach to avoid breaking other things.
|
Other than my comment, code looks good (not tested) 👍 |
|
I've tested this with 1000 users, all of them belonging to same group, sharing every user a folder with a different name with the group. There are no errors in the logs. But when logging with any user it takes a lot of time to login (excessive), and forever to load the files view. |
|
I don't think I can easily reduce the time it takes for login here. But the most important is that the recursion is gone because it would cause crashes. |
|
@SergioBertolinSG did you have XDebug enabled in your test ? If yes then it's good news and likely that the recursion is indeed gone. |
|
No, it is not enabled. |
|
Using same folder name for all users, no problems found. No errors in logs. The performance problem in login and files view I guess it would be the same. This went quicker because I stopped the script before. |
|
I also ran smashbox against this branch and no errors were found. Merging. |
ghost
mentioned this pull request
|
I did a short test and it seems like this also fixed #25884 (Memory issues on command line rescans via occ). |
|
@todestoast thanks for testing. If you had shares on your system, it is likely that "occ files:scan" would also mount shared storages and triggered the recursion that this PR fixes. So yeah, good to hear it fixed that too! |
|
I managed to isolate steps to reproduce this locally, we should add integration tests for these: #25557 (comment) |
|
QA ticket for integration tests: owncloud/QA#350 |
|
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. |
Fixes #25557
Obsoletes #25754 which was branched off v9.1.0 and was missing other sharing fixes that went into stable9.1
@owncloud/sharing @owncloud/filesystem
@SergioBertolinSG can you retest with 1000 users on this PR ?