perf: Cache read only state for pushing steps #7086
Conversation
| if ($isReadOnly === null) { | ||
| $file = $this->getFileForSession($session, $shareToken); | ||
| $isReadOnly = $this->isReadOnly($file, $shareToken); | ||
| $this->cache->set($cacheKey, $isReadOnly, 60 * 5); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
From discussion in the call:
- Listen for share deleted event to clean up sessions when a public share link is revoked
There was a problem hiding this comment.
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.
Signed-off-by: Julius Knorr <[email protected]>
juliusknorr
force-pushed
the
perf/filesystem-setup
branch
2 times, most recently
from
6a6a2ed to
1fa5b84
Compare
|
/backport to stable31 |
This saves file system setup for pushing steps by caching the read only check for 5 minutes.
Ideally we can also do this during sync requests, however there we currently need the etag/mtime of the file to check for conflicts. We may remove this in the future, but for now I'd like to still check that to have a backportable change. Looking into options how we can get that info without a full file system setup. However for now the first commit should already be ok as it is.
Did a local comparison with some larger amount of shares where the file is located in a talk share
Should cut a decent share of the 8% samples of the overall flamegraph:
