Skip to content

perf(base): Stop setting up the FS for every basic auth request #53141

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.

Sign up for GitHub

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

Jump to bottom
Merged
skjnldsv merged 7 commits into from
Jul 11, 2025
Merged

perf(base): Stop setting up the FS for every basic auth request #53141

skjnldsv merged 7 commits into from
Jul 11, 2025

Conversation

@provokateurin
Copy link
Member

@provokateurin provokateurin commented May 27, 2025 •
edited
Loading

Summary

An alternative approach to #52980 as it seems all the code is just dead (let's see what CI says). Probably quite similar to #36589, but I tried to remove as little as possible (so further cleanups would be needed).

Checklist

@provokateurin provokateurin added this to the Nextcloud 32 milestone May 27, 2025
@provokateurin provokateurin mentioned this pull request May 27, 2025
Closed
4 tasks
@provokateurin provokateurin force-pushed the perf/files/setup-fs-basic-auth-request branch 2 times, most recently from 63a9c61 to 0551675 Compare May 27, 2025 13:55
@provokateurin provokateurin changed the title fix(dav): Initialize the FS for the user right after authenticating perf(base): Stop setting up the FS for every basic auth request Jun 2, 2025
@provokateurin provokateurin force-pushed the perf/files/setup-fs-basic-auth-request branch from 0551675 to 9326548 Compare June 2, 2025 12:01
@provokateurin provokateurin added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 2, 2025
@provokateurin provokateurin marked this pull request as ready for review June 2, 2025 12:01
@provokateurin provokateurin requested a review from a team as a code owner June 2, 2025 12:01
@provokateurin provokateurin requested review from Altahrim, juliusknorr, leftybournes, miaulalala, nickvergessen and yemkareems and removed request for a team June 2, 2025 12:01
@provokateurin
Copy link
Member Author

provokateurin commented Jun 2, 2025

Requesting review from the people who did the previous attempts, just to be sure there is nothing wrong...

come-nc
come-nc reviewed Jun 2, 2025
View reviewed changes
@provokateurin
Copy link
Member Author

provokateurin commented Jun 2, 2025

The AvatarController is abusing the cache as it relies on the data being present. Cached data can be removed at any point, so not having a cache must also work (the integration tests don't).

@provokateurin provokateurin force-pushed the perf/files/setup-fs-basic-auth-request branch from 9326548 to 32c331a Compare June 2, 2025 13:44
@provokateurin provokateurin requested a review from come-nc June 2, 2025 13:44
@nickvergessen
Copy link
Member

nickvergessen commented Jun 4, 2025

The AvatarController is abusing the cache as it relies on the data being present. Cached data can be removed at any point, so not having a cache must also work (the integration tests don't).

Which worked before with the file cache as it was simply written to disk.
Could be migrated to appdata / ISimpleFile in the meantime.

@provokateurin provokateurin force-pushed the perf/files/setup-fs-basic-auth-request branch from 32c331a to d92e5e2 Compare July 1, 2025 09:12
@provokateurin provokateurin requested a review from a team as a code owner July 1, 2025 09:12
@provokateurin provokateurin requested review from susnux and removed request for a team July 1, 2025 09:12
@provokateurin provokateurin force-pushed the perf/files/setup-fs-basic-auth-request branch from d92e5e2 to a324c76 Compare July 1, 2025 10:53
@provokateurin
Copy link
Member Author

provokateurin commented Jul 1, 2025

/compile

@nextcloud-command nextcloud-command requested a review from a team as a code owner July 1, 2025 11:06
provokateurin and others added 7 commits July 8, 2025 11:38
@provokateurin
fix(dav): Initialize the FS for the user right after authenticating
689a853 
Signed-off-by: provokateurin <[email protected]>
@provokateurin
perf(base): Stop setting up the FS for every basic auth request
0eae0e3 
Signed-off-by: provokateurin <[email protected]>
@provokateurin
fix(core): Stop abusing the cache for avatar upload
24f7a2e 
Signed-off-by: provokateurin <[email protected]>
@provokateurin
refactor(Server): Deprecate \OCP\ICache service and replace it with a…
57191d4 
… distributed cache

Signed-off-by: provokateurin <[email protected]>
@provokateurin
chore: Remove unused \OC\Cache\File
0d8f983 
Signed-off-by: provokateurin <[email protected]>
@provokateurin
chore: Remove unused \OC\Cache\CappedMemoryCache
9070552 
Signed-off-by: provokateurin <[email protected]>
@nextcloud-command @provokateurin
chore(assets): Recompile assets
caddc8d 
Signed-off-by: nextcloud-command <[email protected]>
@provokateurin provokateurin force-pushed the perf/files/setup-fs-basic-auth-request branch from 017eeb4 to caddc8d Compare July 8, 2025 09:43
@skjnldsv skjnldsv merged commit 6f0255d into master Jul 11, 2025
221 of 243 checks passed
@skjnldsv skjnldsv deleted the perf/files/setup-fs-basic-auth-request branch July 11, 2025 13:25
@provokateurin
Copy link
Member Author

provokateurin commented Jul 11, 2025

@skjnldsv I'm not sure if the CI failure was related, so we might have always red master now...

@skjnldsv
Copy link
Member

skjnldsv commented Jul 11, 2025

@skjnldsv I'm not sure if the CI failure was related, so we might have always red master now...

I'll have a look 👍

@nickvergessen
Copy link
Member

nickvergessen commented Jul 11, 2025

Yeah sounds like it broke all external storages :/

@skjnldsv
Copy link
Member

skjnldsv commented Jul 11, 2025

Dammit 😢
The merge button was green

@skjnldsv
Copy link
Member

skjnldsv commented Jul 11, 2025

So, revert ?

@provokateurin
Copy link
Member Author

provokateurin commented Jul 11, 2025

For me it was red 🤔 Yeah please revert :/

This was referenced Jul 11, 2025
Merged
Merged
@skjnldsv
Copy link
Member

skjnldsv commented Jul 11, 2025

#53920

@provokateurin
Copy link
Member Author

provokateurin commented Jul 11, 2025

@skjnldsv Why did you only revert one commit?

@nickvergessen
Copy link
Member

nickvergessen commented Jul 11, 2025

Because GitHub always only reverts the merge commit

@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@come-nc come-nc come-nc approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@miaulalala miaulalala Awaiting requested review from miaulalala

@susnux susnux Awaiting requested review from susnux susnux is a code owner automatically assigned from nextcloud/server-frontend

@artonge artonge Awaiting requested review from artonge artonge is a code owner automatically assigned from nextcloud/server-frontend

Assignees

No one assigned

Labels

3. to review Waiting for reviews performance 🚀

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

Basic auth requests always set up the filesystem

7 participants

@provokateurin @nickvergessen @skjnldsv @juliusknorr @come-nc @nextcloud-command