(dev/core#174) Forms/Sessions - Store state via Civi::cache('session')

[totten]

Overview

When using forms based on CiviQuickForm (specifically CRM_Core_Controller), CRM_Core_Session stores form-state via CRM_Core_BAO_Cache::storeSessionToCache and ::restoreSessionFromCache, which in turn calls CRM_Core_BAO_Cache::setItem() and ::getItem().

However, using CRM_Core_BAO_Cache::setItem() and ::getItem() means that all session state must be written to MySQL. For dev/core#174, we seek the option to store via Redis/Memcache.

Before

• (a) Form/session state is always stored via CRM_Core_BAO_Cache::setItem() and civicrm_cache table.
• (b) To ensure that old sessions are periodically purged, there is special purpose logic that accesses civicrm_cache (roughly delete where group_name=Sessions and created_date < now()-ttl).
• (c) On Memcache/Redis-enabled systems, the cache server functions as an extra tier. The DB provides canonical storage for form/session state.

After

• (a) Form/session state is stored via CRM_Utils_CacheInterface.
  • On a typical server, this defaults to CRM_Utils_Cache_SqlGroup and civicrm_cache table.
• (b) To ensure that old sessions are periodically purged, the call to CRM_Utils_CacheInterface::set() specifies a TTL.
  • It is the responsibility of the cache driver to handle TTLs. With (dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC #12360, TTL's are supported in ArrayCache, SqlGroup, and Redis.
• (c) On Memcache/Redis-enabled systems, the cache server provides canonical storage for form/session state.

Comments

This should generally be a drop-in replacement (i.e. the TTLs and performance should be the same or better), but there are a couple considerations which leave me ambivalent:

1. The CRM_Utils_Cache_Memcache and CRM_Utils_Cache_Memcached drivers don't currently support TTL. (Addressing should be somewhat rote after fixing the other cache drivers.) (Resolved)
2. If an administrator currently uses Memcache/Redis, this is a subtle policy change. (In the Before/After, compare item (c).) The significance of this depends a lot on how much you care about the performance-implications of MySQL writes -- and on how reliable your cache-service is. If your cache-service is frequently restarted, or if it's scaled/partitioned in weird ways, then you might prefer the old policy.

If the second point is an issue, then we'd still write this patch in the same basic way, but we'd need another configuration-management patch so that sysadmins can influence the construction of the cache.session service. Basically, these are interesting policies for cache.session:

// Store only in Redis/Memcache -- or only in MySQL -- or only in ArrayCache.
$sessionCache = CRM_Utils_Cache::create(array(
  'name' => 'CiviCRM Sessions',
  'type' => array('*memory*', 'SqlGroup', 'ArrayCache'),
));

// Strictly store sessions only in MySQL
$sessionCache = CRM_Utils_Cache::create(array(
  'name' => 'CiviCRM Sessions',
  'type' => array('SqlGroup'),
));

// Store canonically in MySQL, but provide other tiers. Same as old policy.
$sql = CRM_Utils_Cache::create(array(
  'name' => 'CiviCRM Sessions',
  'type' => array('SqlGroup'),
));
$memory  = CRM_Utils_Cache::create(array(
  'name' => 'CiviCRM Sessions',
  'type' => array('*memory*', 'ArrayCache),
));
$sessionCache = new CRM_Utils_Cache_Chained([$memory, $sql]);

In an ideal world, it'd be fine to expose those configuration options. Practically speaking, we would need (a) an implementation of CRM_Utils_Cache_Chained and (b) a configuration mechanism. The thing I'm not sure about is how much to care about the above. Treat them as blockers on the critical path (to yield stricter drop-in compatibbility) -- or as nice-to-haves that someone can figure out in a future release?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

One consideration I'm mulling -- today, if one clears the civicrm_cache table (i.e. by calling CRM_Core_Config::clearDBCache() ==> TRUNCATE TABLE civicrm_cache), then it has the effect of destroying any active sessions/forms. So, if we wanted strictly equivalent behavior from a business-logic perspective, we'd want the patch to go a bit further -- calling Civi::cache('session')->clear() at the same time that it does clearDBCache().

On the other hand, some of them don't entirely make sense to me, and I remember some feedback (from @mlutfy?) that the system is a bit too zealous about clearing sessions.

Rather than arguing theory, here's a breakdown of code which has traditionally called clearDBCache() (and implicitly removed all active sessions):

https://docs.google.com/spreadsheets/d/1FxuIvr2noelBvhu5eja9_ps3YUWnkmGhqijBO3gH8Po/edit?usp=sharing

Which of these should now call Civi::cache('session')->clear()? I've taken a stab at answering that, but I'd be interested if someone else -- @seamuslee001 @mlutfy (or anyone else) -- has any opinions.

(Update: I've included an example commit to sprinkle in some clear() calls - but I'm happy to revise w/discussion.)

[mlutfy]

My main worry with cache-clearing, was that folks using CiviCase can often end up entering data for 1h. They might be talking with someone, take a few notes, then submit the form after 1h, and then the cache may have been cleared so their QF key is lost.

I've been debugging this issue again, since it popped up again after upgrading a 4.6 client to 4.7. Seeing places like CRM_Event_BAO_Event::add() do a CRM_Utils_System::flushCache() is a bit of a headache to debug.

In any case, it seems like you're leaning to reduce the qty of full cache-clear, and I support that.

I suspect that the code did a truncate was because MySQL can be pretty slow to delete things on non-keyed data, and one might want to be careful of how many keys to add on that kind of table with data that changes often?

[totten]

Rebased. (One of the dependencies was merged.)

In line with a chat with @seamuslee001 (MM/GDoc), I've added a commit to trigger session-clear in some of those spots (including the debated one CRM_Admin_Form_Setting).

Seeing places like CRM_Event_BAO_Event::add() do a CRM_Utils_System::flushCache() is a bit of a headache to debug.

In any case, it seems like you're leaning to reduce the qty of full cache-clear, and I support that.

Yeah, I really agree. TBH, I'm fiddling at the edges here. The various cache clearing functions functions feel mysterious because they're all over the map (stylistically and functionally). We probably need more organizational tooling (e.g. ability to list all caches; e.g. ability to tag related caches). That would be interesting to work on sometime, but I'm gonna punt it for the moment.

[eileenmcnaughton]

I guess this needs a rebase now

[totten]

Yup. Rebased and updated title/description to remove references to the PR-dependencies.

[eileenmcnaughton]

@totten So I've read through this & I feel fairly comfortable about merging it. You raise that it will cause a behaviour change for Redis & Memcache users (not apc?)

I feel like we know @herbdool uses Redis & @lcdservices uses Memcached & possibly Community Services do.

I'm inclined to merge this & email the dev list and / or post a blog to try to find out who actually uses these & try to get them to test the rc.

Annecdotally I know some have tried & given up on Memcache so I think we might be dealing with something that is not well used because it doesn't work all that well at the moment.

[eileenmcnaughton]

@adixon @lcdservices @herbdool I'm merging this so it will be in 5.4 rc - if you have any chance to test 5.4 rc on Redis / Memcache that would be great. If the edge case @totten describes turns out to be a real thing then we will do something before 5.4 is cut to mitigate

[herbdool]

I've checked code and fine with it. If I get a chance to test I'll let you know here.