(dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC #12360

totten · 2018-06-24T01:39:42Z

Overview

#12342 provides nominal compliance with PSR-16; however, some drivers raise exceptions or warnings for new options . This PR ensures more complete support for PSR-16 in three drivers (ArrayCache, SqlGroup, and Redis).

(Depends: The patches here work/are cogent, but I've flagged it "Blocked" because it depends/includes other open PRs: #12342, #12348, #12354)

Before

ArrayCache, SqlGroup, Redis, Memcache, Memcached, and APCcache are not tested for compliance -- and would not pass if they were because:
- $cache->set(...$ttl) would throw an error any custom TTL.
- $cache->get(...$default) woudl throw an error for any not-NULL default.
- Cache-keys are not validated.
- Some variants of $cache->set() / $cache->get() handle objects in ways that are unsafe.
- In some variants, $cache->flush() is overzealous (Memcache); in others, underzealous (APCcache).

After

ArrayCache, SqlGroup, Redis, Memcache, Memcached, and APCcache have been fixed to comply (per SimpleCacheTest), and they comply.

Technical Details

SqlGroup is substantially rewritten. It no longer wraps around CRM_Core_BAO_Cache - instead, it does SQL queries which are similar in nature and uses its own front-cache.
- The interface and implementation of CRM_Core_BAO_Cache do not support TTL/expiration. Adding support effectively requires patching every function to respect TTL in reading+writing both DB and thread-local front-cache. And in the long-run, it's better to maintain that kind of logic in framework that is standard compliant.
- I was concerned about a theoretical issue -- if CRM_Core_BAO_Cache and CRM_Utils_Cache_SqlGroup wrap the same DB table but have different front-caches, and if you tried accessing the same records through both interfaces, then there could be cache-coherence bugs. However, if we consider the audit of caches, it becomes less of a concern. Why? Because the use-cases are XOR. (Ex: The dashboard cache always uses BAO to read/write cache-rows. Ex: The js_strings cache always uses OOP to read/write cache-rows.)
- At some point, we may want to fully flip the relationship around (e.g. BAO_Cache could just be a thin wrapper for SqlGroup which we provide for backward compatibility). However, that will make even bigger PR that's harder to review en masse.
The test coverage for SqlGroup and ArrayCache should work intuitively. For Redis, Memcache, and Memcached, the test needs a particular environment to run -- which isn't universally available. The ConfiguredMemoryTest will run against whatever cache service is enabled in civicrm.settings.php -- and if no service is configured, it will be skipped. (APCcache is similar, although the specific environmental requirements differ.) For the tests which don't run in the CI environment, I've confirmed they pass locally.

civibot · 2018-06-24T01:39:44Z

(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).

seamuslee001 · 2018-06-24T04:34:21Z

CRM/Utils/Cache/SqlGroup.php

+    //    CRM_Core_BAO_Cache::setItem($value, $this->group, $key, $this->componentID);
+    $lock = Civi::lockManager()->acquire("cache.{$this->group}_{$key}._null");
+    if (!$lock->isAcquired()) {
+      CRM_Core_Error::fatal();


@eileenmcnaughton @totten should we throw exception here rather than fatal?

@seamuslee001 Probably. Thinking out loud:

The system is generally simpler/clearer with exceptions.

If there's anyone using SqlGroup who's trying to catch/handle problems, they could be surprised by a switch from fatal to exception.

OTOH, very few things try to handle fatals. (Because they're awkward to handle.) And when they are handled, that's by converting the fatal to an exception. (Not strictly so - but in practical terms, that's all I've ever seen or heard of.)

Overall, the previous contract for CRM_Utils_Cache_Interface was unclear about how errors would be reported. PSR-16 pushes us to standardize (i.e. returning booleans and throwing particular exception-interfaces).

I suspect real-world error-handling behaviors in CRM/Utils/Cache/* have been pretty varied (depending on the particular driver/configuration, you might get exceptions or fatals or FALSE return values). Any problem that rises from "making the error handling more consistent" is likely to be in the category of "the problem existed before but manifested differently".

We don't often get to highlight the cache-subsystem as an area of change. It may be net-gentler on downstream to rip-off this particular bandaid rather than kick it down the road.

So... yes, I'll update to use the exception instead.

@seamuslee001 updated and squashed revisions to report exceptions instead of fatals. This also adds a concrete class to implement \Psr\SimpleCache\CacheException.

totten · 2018-06-25T21:36:39Z

jenkins, test this please

totten · 2018-06-25T23:46:35Z

Rebased with some updates:

Fixed typo in various commit messages (173 vs 174)
Added (dev/core#174) CRM_Utils_Cache::assertValidKey - Relax enforcement at runtime (d6d6320). This doesn't replace (dev/core#174) Cache-keys for CRM_Utils_Cache_* should avoid reserved chars #12348, but it should fail more gracefully in production if there are oddball scenarios that we haven't found/fixed.

seamuslee001 · 2018-06-27T21:20:00Z

@totten are you able to rebase this now please?

seamuslee001 · 2018-06-27T22:16:23Z

CRM/Utils/Cache/Memcached.php

+        throw new CRM_Utils_Cache_CacheException("Memcached::set($key) failed: " . $this->_cache->getResultMessage());
+      }
+      else {
+        Civi::log()->error("Redis set ($key) failed: " . $this->_cache->getResultMessage());


@totten i think this should be Memcached here

Thanks. Fixed and squashed.

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 civicrm#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.

eileenmcnaughton · 2018-06-29T18:35:32Z

I wish we had stats on the use of these - my guess is it would be better to move some out to an extension at some point

eileenmcnaughton · 2018-06-29T18:36:11Z

CRM/Utils/Cache.php

+   * @throws \CRM_Utils_Cache_InvalidArgumentException
+   */
+  public static function assertValidKey($key) {
+    $strict = CRM_Utils_Constant::value('CIVICRM_PSR16_STRICT', FALSE) || defined('CIVICRM_TEST');


I like this

eileenmcnaughton · 2018-06-29T18:45:05Z

CRM/Utils/Cache/APCcache.php

-    if ($default !== NULL) {
-      throw new \RuntimeException("FIXME: " . __CLASS__ . "::get() only supports NULL default");
+    CRM_Utils_Cache::assertValidKey($key);
+    $result = apc_fetch($this->_prefix . $key, $success);


$success does not seem to be defined

That sounds like some metaphysical management widsom. ;)

More seriously, $success is actually an output of this line. apc_fetch($key, &$success) sets the value of $success to indicate whether it worked.

eileenmcnaughton · 2018-06-29T21:34:18Z

CRM/Utils/Cache/NaiveMultipleTrait.php

+   * @throws \CRM_Utils_Cache_InvalidArgumentException
+   */
+  private function assertIterable($func, $keys) {
+    if (!is_array($keys) || $keys instanceof Traversable) {


I feel like this should be reversed ie

While you cannot implement this interface, you can use it in your checks to determine if something is usable in for each. Here is what I use if I'm expecting something that must be iterable via foreach. in the example not !items

http://php.net/manual/en/class.traversable.php

Oops, good catch! This explains why testGetMultipleWithGenerator(),testSetMultipleWithGenerator(), and testDeleteMultipleGenerator() weren't working. :)

I've pushed up a patch to fix assertIterable and re-enable those tests.

… BAO multitier cache.

There are two drivers, `CRM_Utils_Memcache` and `CRM_Utils_Memcached`. It's nice to update them in tandem (with similar design decisions). If an admin admin is experimenting/debugging, this consistency makes it easier to switch between drivers. (Cache data written by one driver can be read by the other driver.) In addition to the standard PSR-16-style changes, there are a couple changes in how data is formatted when written to memcache: * To allow support for targetted `flush()`ing (one prefix at a time), we update the naming convention per https://github.com/memcached/memcached/wiki/ProgrammingTricks#deleting-by-namespace This means that a typical key includes a bucket-revision code: * BEFORE: `<site-prefix>/<bucket-prefix>/<item-key>` (`dmaster/default/mykey`) * BEFORE: `<site-prefix>/<bucket-prefix>/<bucket-revision>/<item-key>` (`dmaster/default/5b33011fea555/mykey`) * Values are `serialize()`d. This resolves an ambiguity where `Memcache::get()` does not let us know if it returns `FALSE` because there's an error because that's the stored value. By serializing, those scenarios can be distinguished. * `get(...) === FALSE` means "item was not found" * `get(...) === serialize(FALSE)` means "item was found with value FALSE"

One noteable quirk is that APC retains expired (per TTL) records until the following page-request. To comply with the test suite, we have to double-check the expiration time.

totten · 2018-07-02T05:06:06Z

Most of this has been split-out to smaller PRs. There was one last bit, which I've now put in #12389, so we'll close this one.

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 civicrm#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.

civicrm-builder added the master label Jun 24, 2018

seamuslee001 reviewed Jun 24, 2018

View reviewed changes

totten force-pushed the master-psr16-full branch from d903350 to 319f6f9 Compare June 24, 2018 23:20

totten force-pushed the master-psr16-full branch from 319f6f9 to 8d28c09 Compare June 25, 2018 23:30

totten added a commit to totten/civicrm-core that referenced this pull request Jun 26, 2018

(REVERT) Cumulative changes for master-psr16-full / civicrm#12360

3b4469d

totten force-pushed the master-psr16-full branch from 8d28c09 to 269d50c Compare June 26, 2018 00:54

totten added a commit to totten/civicrm-core that referenced this pull request Jun 26, 2018

(REVERT) Cumulative changes for master-psr16-full / civicrm#12360

004f2ab

totten changed the title ~~(WIP) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis~~ (Blocked) (dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis Jun 26, 2018

totten mentioned this pull request Jun 26, 2018

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

Merged

totten force-pushed the master-psr16-full branch 3 times, most recently from ad7a6dc to 975a7fe Compare June 27, 2018 03:32

totten changed the title ~~(Blocked) (dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis~~ (Blocked) (dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC Jun 27, 2018

totten force-pushed the master-psr16-full branch from 375ff42 to ec7313e Compare June 27, 2018 06:57

totten force-pushed the master-psr16-full branch from ec7313e to 5b05b32 Compare June 27, 2018 21:33

seamuslee001 reviewed Jun 27, 2018

View reviewed changes

totten force-pushed the master-psr16-full branch 3 times, most recently from 8602436 to 5914e0e Compare June 28, 2018 16:30

totten changed the title ~~(Blocked) (dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC~~ (dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC Jun 28, 2018

eileenmcnaughton reviewed Jun 29, 2018

View reviewed changes

totten mentioned this pull request Jun 29, 2018

(dev/core#174) Add various utilities to support PSR-16 #12376

Merged

totten added 7 commits June 30, 2018 12:55

(dev/core#174) ArrayCache - Updates to comply with PSR-16

564999a

(dev/core#174) Redis - Updates to comply with PSR-16

14e43b5

(dev/core#174) SqlGroup - Rewrite with PSR-16 and TTL support. Bypass…

8307105

… BAO multitier cache.

(dev/core#174) CRM_Core_BAO_Cache - Periodically flush expired records

f687a41

(dev/core#174) APCcache - Updates to comply with PSR-16

2a7f3fe

One noteable quirk is that APC retains expired (per TTL) records until the following page-request. To comply with the test suite, we have to double-check the expiration time.

(dev/core#174) Add test to prevent cross-effects among caches

b6d78ec

totten force-pushed the master-psr16-full branch from aadd265 to b6d78ec Compare June 30, 2018 19:55

eileenmcnaughton mentioned this pull request Jun 30, 2018

(dev/core#174) Full PSR-16 compliance for ArrayCache, Redis #12378

Merged

totten closed this Jul 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC #12360

(dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC #12360

totten commented Jun 24, 2018 •

edited

Loading

civibot bot commented Jun 24, 2018

seamuslee001 Jun 24, 2018

totten Jun 24, 2018

totten Jun 24, 2018

totten commented Jun 25, 2018

totten commented Jun 25, 2018

seamuslee001 commented Jun 27, 2018

seamuslee001 Jun 27, 2018

totten Jun 27, 2018

eileenmcnaughton commented Jun 29, 2018

eileenmcnaughton Jun 29, 2018

eileenmcnaughton Jun 29, 2018

totten Jun 29, 2018

eileenmcnaughton Jun 29, 2018

totten Jun 29, 2018

totten commented Jul 2, 2018

(dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC #12360

(dev/core#174) Full PSR-16 compliance for ArrayCache, SqlGroup, Redis, Memcache, APC #12360

Conversation

totten commented Jun 24, 2018 • edited Loading

Overview

Before

After

Technical Details

civibot bot commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totten commented Jun 25, 2018

totten commented Jun 25, 2018

seamuslee001 commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totten commented Jul 2, 2018

totten commented Jun 24, 2018 •

edited

Loading