(REF) Add CRM_Utils_Cache::nack(). Use it for NaiveHasTrait. #13500

totten · 2019-01-23T21:21:52Z

Overview

When using PSR-16 to read a cached value, the $cache->get($key) will a return a default value on cache-miss, so it's hard to know if you've gotten a genuine value from the cache or just a default. If you need to know that, then it requires a second call to $cache->has($key).

For some scenarios (esp for upcoming #13496 but also for existing NaiveHasTrait), it's important to know if you have a default or a cache-miss, and the code is central enough that we should avoid unnecesary roundtrips.

Before

Most cache drivers in Civi mix-in NaiveHasTrait; that implementation of $cache->has() requires two round-trips.
If you want to do a $cache->has() and $cache->get(), then there are typically three round-trips.

After

Most cache drivers in Civi mix-in NaiveHasTrait; that implementation of $cache->has() only requires one round-trip.

If you want to do a $cache->has() and $cache->get(), it can be handled with one round-trip:

$nack = CRM_Utils_Cache::nack();
$value = $cache->get('foo', $nack);
echo ($value === $nack) ? "Cache contains [$value]" : "Cache has no value".

…in PSR-16

The `NaiveHasTrait` is a generic implementation of PSR-16 `has()` which builds on the logic of PSR-16 `get()`. This reduces I/O for `has()`. Before ------ * Each call to `has()` triggers two calls to `get()`. After ----- * Each call to `has()` triggers one call to `get()`. Comments -------- The correctness of this stems from the uniqueness of the `$nack` value. To wit: if you always use the same constant (e.g. `NULL` or `0` or `''` or `'no-value'`) to signify a cache-miss, then it's trivial to produce a collision/incorrect-result. (Simply store that constant.) But if the value is a sufficiently unique nonce, then it becomes impractical to produce a collision/incorrect-result.

civibot · 2019-01-23T21:21:54Z

(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 · 2019-01-23T23:59:05Z

Note for reviewers: the function NaiveHasTrait::has($key) is inherited by various classes under CRM/Utils/Cache/*.php. These classes have shared unit-test coverage (via E2E_Cache_* classes which extend \Cache\IntegrationTests\LegacySimpleCacheTest). That class originates with the php-cache project and provides pretty good test-coverage for function has($key) (and other parts of the CacheInterface).

TLDR: If NaiveHasTrait::has() is broken, then there should be lots of red-flags in the test suite.

eileenmcnaughton · 2019-01-30T00:42:42Z

This makes sense architecturally & has test coverage as described. The has function is overriden in the MemCache implementation but I can't actually find any place that calls this.

Seems OK to merge as if lays the groundwork for later things

This is a follow-up to civicrm#13500. Before ------ * `CRM_Utils_Cache::nack()` returns an array with a value named `nack` * The value returned is somewhat unique -- a random value is generated once per page-view After ----- * `CRM_Utils_Cache::nack()` returns a string * The value returned is more unique -- combining that random value (per page-view) and an offset (per invocation) Comments -------- * The code was originally written with the intent of returning a string. However, there was a slight copy-paste error which caused it to return an array (which contained that string). It functioned correctly anyway, but it feels weird when reading/inspecting. * The more unique the nack-value is, the more correct the nack-checking pattern is. Appending a static-counter is a simple, fast way to provide stronger uniqueness within a page-view. * There may be some obscure edge-cases in which the previous pattern was not sufficiently unique -- e.g. combining tiers-of-tiers or decorators-of-decorators. I haven't verified that, but it seems unimportant given that the static-counter is so straightforward. * In `NaiveHasTrait`, the extra `ht` suffix was an attempt to increase the uniquness. However, the static-counter seems better.

This is a follow-up to civicrm#13500. Before ------ * `CRM_Utils_Cache::nack()` returns an array with a value named `nack`. * The value returned is somewhat unique -- a random value is generated once per page-view. * There is no explicit/direct unit-test. After ----- * `CRM_Utils_Cache::nack()` returns a string. * The value returned is more unique -- combining that random value (per page-view) and an offset (per invocation). * There is an explicit/direct unit-test. Comments -------- * The code was originally written with the intent of returning a string. However, there was a slight copy-paste error which caused it to return an array (which contained that string). Functionally, that worked (because it was serializable and met the same minimum uniqueness constraint), but it's weird to read/inspect, and we should change quickly before something else locks-in the odd structure. * The more unique the nack-value is, the more correct the nack-checking pattern is. Appending a static-counter is a simple, fast way to provide stronger uniqueness within a page-view. * There may be some obscure edge-cases in which the previous pattern was not sufficiently unique -- e.g. combining tiers-of-tiers or decorators-of-decorators. I haven't verified that, but it seems unimportant given that the static-counter is so straightforward. * In `NaiveHasTrait`, the extra `ht` suffix was an attempt to increase the uniquness. However, the static-counter seems better.

totten · 2019-01-30T05:48:19Z

CRM/Utils/Cache.php

+    if (!isset(Civi::$statics[__CLASS__]['nack'])) {
+      Civi::$statics[__CLASS__]['nack'] = 'NACK:' . md5(CRM_Utils_Request::id() . CIVICRM_SITE_KEY . CIVICRM_DSN . mt_rand(0, 10000));
+    }
+    return Civi::$statics[__CLASS__];


Doh. Line 286 should be like lines 283+284 --specially address the nack item (rather than the array of statics held for this class). This worked in testing anyway, albeit for the wrong reason. Correction: #13514

totten added 2 commits January 23, 2019 13:03

CRM_Utils_Cache::nack() - Add helper for combining get()+has() calls …

e0ba77f

…in PSR-16

civibot bot added the master label Jan 23, 2019

totten mentioned this pull request Jan 23, 2019

(dev/core#635) Implement local array-cache for use with Redis/Memcache #13496

Merged

totten mentioned this pull request Jan 24, 2019

(dev/core#635) Deprecate CRM_Core_BAO_Cache for I/O. Optionally redirect I/O to Redis or Memcache. #13489

Merged

eileenmcnaughton merged commit 845ed62 into civicrm:master Jan 30, 2019

totten deleted the master-nack branch January 30, 2019 05:08

totten mentioned this pull request Jan 30, 2019

(dev/core#635) CRM_Utils_Cache::nack() - Fix format #13514

Merged

totten commented Jan 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(REF) Add CRM_Utils_Cache::nack(). Use it for NaiveHasTrait. #13500

(REF) Add CRM_Utils_Cache::nack(). Use it for NaiveHasTrait. #13500

totten commented Jan 23, 2019

civibot bot commented Jan 23, 2019

totten commented Jan 23, 2019 •

edited

Loading

eileenmcnaughton commented Jan 30, 2019

totten Jan 30, 2019

(REF) Add CRM_Utils_Cache::nack(). Use it for NaiveHasTrait. #13500

(REF) Add CRM_Utils_Cache::nack(). Use it for NaiveHasTrait. #13500

Conversation

totten commented Jan 23, 2019

Overview

Before

After

civibot bot commented Jan 23, 2019

totten commented Jan 23, 2019 • edited Loading

eileenmcnaughton commented Jan 30, 2019

totten Jan 30, 2019

Choose a reason for hiding this comment

totten commented Jan 23, 2019 •

edited

Loading