Skip to content

perf: Cache read only state for pushing steps #7086

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

Merged
juliusknorr merged 1 commit into from
Apr 22, 2025
Merged

perf: Cache read only state for pushing steps #7086

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/Service/DocumentService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public function writeDocumentState(int $documentId, string $content): void {
*/
public function addStep(Document $document, Session $session, array $steps, int $version, ?string $shareToken): array {
$documentId = $session->getDocumentId();
$readOnly = $this->isReadOnly($this->getFileForSession($session, $shareToken), $shareToken);
$readOnly = $this->isReadOnlyCached($session, $shareToken);
$stepsToInsert = [];
$stepsIncludeQuery = false;
$documentState = null;
Expand Down Expand Up @@ -564,6 +564,18 @@ public function getFileByShareToken(string $shareToken, ?string $path = null): F
throw new \InvalidArgumentException('No proper share data');
}

public function isReadOnlyCached(Session $session, ?string $shareToken = null): bool {
$cacheKey = 'read-only-' . $session->getId();
$isReadOnly = $this->cache->get($cacheKey);
if ($isReadOnly === null) {
$file = $this->getFileForSession($session, $shareToken);
$isReadOnly = $this->isReadOnly($file, $shareToken);
$this->cache->set($cacheKey, $isReadOnly, 60 * 5);
Copy link
Member

@mejo- mejo- Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought a bit about the possible race conditions created by caching the read-only permissions for five minutes:

  • If the session user is the file owner, permissions should not change on the fly, except maybe when the whole filesystem (storage/mountpoint???) is rendered read-only. Maybe still warrants an event listener to invalidate the cache?
  • If the session user is e.g. part of a collective/groupfolder, permissions might change, so I wonder if we need some kind of event listeners to invalidate the cache in this case.
  • Same if the session user access the file through a share: the cached permissions might become invalid which would mean privilege escalation (or downgrade) for a max of five minutes - so I guess we need to take care of invalidating the cache here.
  • Looking at isReadOnly() in this class we also consider file locks by other apps than text as a reason to render it read-only. Here the likeliness is even higher that we run into race conditions, no?

Maybe I'm missing something. If we want to go without invalidation and just live with possible short periods of race conditions, I'd vouch for a much shorter cache lifespan than five minutes, e.g. like 30 seconds.

Copy link
Member Author

@juliusknorr juliusknorr Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From discussion in the call:

  • Listen for share deleted event to clean up sessions when a public share link is revoked

Copy link
Member Author

@juliusknorr juliusknorr Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the session user is the file owner, permissions should not change on the fly, except maybe when the whole filesystem (storage/mountpoint???) is rendered read-only. Maybe still warrants an event listener to invalidate the cache?
If the session user is e.g. part of a collective/groupfolder, permissions might change, so I wonder if we need some kind of event listeners to invalidate the cache in this case.

It is not that easy unfortunately. We may have storages (groupfolders/external storage) where the current user is always considered as the owner, no matter that in theory it would be a shared ownership. We also do not have events or hooks that would notify us on permisssion change on the mount or file level without setting up the filesystem.

Looking at isReadOnly() in this class we also consider file locks by other apps than text as a reason to render it read-only. Here the likeliness is even higher that we run into race conditions, no?

No, this is only relevant when opening a file (creating the text document with the first session). Otherwise text will own a lock so there is no need to check again.

Same if the session user access the file through a share: the cached permissions might become invalid which would mean privilege escalation (or downgrade) for a max of five minutes - so I guess we need to take care of invalidating the cache here.

Given with other editing options like Collabora (where WOPI as a protocol by design does not check back further permissions until the file is saved) using a 5 minute delay is still quite acceptable in my opinion.

Listen for share deleted event to clean up sessions when a public share link is revoked

I've been testing this a bit more and would consider this a general enhancement, currently also on main things behave odd if you change permissions or remove the share while editing. I'm all in for improving but would push this change isolated from that. As a follow up I would then consider not invalidating the cache but completely revoking the text session for cases where it makes sense. I currently can only think of a share removal or update that we can properly catch. Not sure about others.

return $isReadOnly;
}

return $isReadOnly;
}

public function isReadOnly(File $file, ?string $token): bool {
$readOnly = true;
Expand Down
Loading