Page MenuHomekolab.org

Remove folder typedata in-memory cache
ClosedPublic

Authored by mollekopf on Jan 21 2021, 10:48 AM.

Details

Summary

This caused the syncroton Ping command to miss new task folders.
Because the Ping command is long running we use:

rcube::get_instance()->get_storage()->clear_cache('mailboxes', true);

To clear the cache before listing the folders. However, this doesn't
clear the in-memory cache in kolab_storage.

In list_folders we then ended up in the branch returning the folder list directly from
that outdated in-memory cache and thus missed the update.

Since we already cache the folder list and metadata it doesn't seem necessary to
cache it again. Removing it thus gives us a single place to invalidate it.

Diff Detail

Repository
rRPK roundcubemail-plugins-kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mollekopf requested review of this revision.Jan 21 2021, 10:48 AM
mollekopf created this revision.

See https://bifrost.kolabsystems.com/T408794 for context. I verified that the RC UI continues to function (especially activesync and folder config),
so it doesn't brake anything, and that the Ping works better for https://bifrost.kolabsystems.com/T408794

With this change METADATA for every folder will be asked separately. This definitely will have performance implications. Maybe a better approach would be to add a method to reset kolab_storage::$typedata (and other cache-like data if needed). And call that method from syncroton's kolab_sync::sleep() or kolab_sync_backend_folder::hasHierarchyChanges().

With this change METADATA for every folder will be asked separately. This definitely will have performance implications. Maybe a better approach would be to add a method to reset kolab_storage::$typedata (and other cache-like data if needed). And call that method from syncroton's kolab_sync::sleep() or kolab_sync_backend_folder::hasHierarchyChanges().

How about we call self::folders_typedata($prefix); to populate the cache, but then user folder_type() anyways to fetch missing entries (it still uses the cache if available)?

How about we call self::folders_typedata($prefix); to populate the cache, but then user folder_type() anyways to fetch missing entries (it still uses the cache if available)?

I don't like it for performance reasons, because if we know the folder has no type then why would you bother with a METADATA request? This would create many redundant requests when listing mail folders. And I'm not sure it would solve your issue.

How about we call self::folders_typedata($prefix); to populate the cache, but then user folder_type() anyways to fetch missing entries (it still uses the cache if available)?

I don't like it for performance reasons, because if we know the folder has no type then why would you bother with a METADATA request? This would create many redundant requests when listing mail folders. And I'm not sure it would solve your issue.

I suppose there are multiple parts to it:

  • Line 873: Removing the shortcut for groupware folders has the effect that we list folders from the roundcube cache consistently, also for groupware folders, thus allowing new folders being discovered (at the cost of the folder listing)
  • Line 898: Using folder_type instead of directly accessing $folderdata just has the effect that we first attempt to get the data from the cache, but then also fetch it if it's not existing (thus solving my problem).

IMO it would be better to make sure that the one cache in roundcube is good enough for performance and avoid extra caching layers that need to be invalidated separately, but if you prefer something non-intrusive we can also do something like: https://git.kolab.org/D2155

If you think that this internal cache is not really needed then go this way and remove use of self::$typedata. In rare cases when we list folders for all folder types separately in a single request, we'll end up with several requests to the Roundcube imap cache, instead of one. I'm afraid that we might call folder_typedata() or folder_type() methods quite a lot, but I didn't check that, so I'm not sure.

mollekopf updated this revision to Diff 5965.Jan 22 2021, 3:08 PM
mollekopf retitled this revision from Avoid listing folders from an outdated in-memory cache to Remove folder typedata in-memory cache.
mollekopf edited the summary of this revision. (Show Details)

Removed the in-memory cache instead.

I think this should be ok for syncroton/chwala.
There's a couple of calls in kolab_delegation_engine, and plenty in kolab_folders,
so I can't really tell if the rc cache performs well enough for all those cases.

machniak accepted this revision.Jan 26 2021, 11:47 AM
This revision is now accepted and ready to land.Jan 26 2021, 11:47 AM
This revision was automatically updated to reflect the committed changes.