From 13ca7ebf46b449fcefa51deee50f6d88c08493c4 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Thu, 5 Jul 2018 17:21:44 -0700
Subject: [PATCH] (dev/core#174) CRM_Utils_Cache_SqlGroup - Refine trivial TTL
 handling to stabilize tests

Since incorporting `E2E_Cache_SqlGroupTest`, we've observed periodic flakiness in some
of the test-runs around code like this:

```php
  $cache->set("key", "value", 1);
  $get = $cache->get("key");
```

This makes the tests more reliable by refining the way `SqlGroup` handles trivially short TTLs.

Before
----------------------------------------
Cache items with `$ttl=1` may be expired before you ever get a chance to read them.

After
----------------------------------------
Cache items with `$ttl=1` may sometimes last slightly longer than a second
(if recorded late in the clock's current second), which improves the odds that
you'll be able to read the cache.

Technical Details
----------------------------------------
To reproduce the problem, run this script a few times via `cv scr`:

```php
<?php
$cache = CRM_Utils_Cache::create([
  'name' => 'tmp-cache',
  'type' => ['SqlGroup']
]);

for ($i = 0; $i < 500; $i++) {
  $cache->set("foo_$i", 'hi', 1);
  $get = $cache->get("foo_$i");
  if ($get !== 'hi') {
    echo "miss! ($i)\n";
  } else {
    echo ".";
  }
}
```

Given that `$cache->set()` and `$cache->get()` are right next to each other,
you would expect (and the test authors did evidently expect) that cache would be valid
long enough to read content. And yet we periodically have a cache-miss (particularly with
`SqlGroup`, which is a bit slower to handle writes).

For example, suppose `microtime()===100.95`.  If you compute expiration as
`$expires=time()+$ttl`, then `$expires===101`.  However, this means that
the cache's effective validity period is more like `~0.05` seconds rather
than a full second.

A more correct solution would be to track expiration times as microseconds.
However, that's a bigger patch, and we don't really need that level of
precision -- typical TTLs for us are more like "a week" or "a day" or "an
hour" or *maybe* "a minute".  The `$ttl=1` is a trivial scenario that only
makes sense for unit-testing.

This patch just rounds up the calculation of `$expires` -- in our
hypothetical at `microtime()===100.95`, that makes `$expires===102` and an
effective validity period of `~1.05` seconds.
---
 CRM/Utils/Cache/SqlGroup.php | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/CRM/Utils/Cache/SqlGroup.php b/CRM/Utils/Cache/SqlGroup.php
index aa4d15e4724..beaf5fd1ddb 100644
--- a/CRM/Utils/Cache/SqlGroup.php
+++ b/CRM/Utils/Cache/SqlGroup.php
@@ -118,8 +118,12 @@ public function set($key, $value, $ttl = NULL) {
       throw new \CRM_Utils_Cache_CacheException("SqlGroup: Failed to acquire lock on cache key.");
     }
 
+    if (is_int($ttl) && $ttl <= 0) {
+      return $this->delete($key);
+    }
+
     $dataExists = CRM_Core_DAO::singleValueQuery("SELECT COUNT(*) FROM {$this->table} WHERE {$this->where($key)}");
-    $expires = CRM_Utils_Date::convertCacheTtlToExpires($ttl, self::DEFAULT_TTL);
+    $expires = round(microtime(1)) + CRM_Utils_Date::convertCacheTtl($ttl, self::DEFAULT_TTL);
 
     $dataSerialized = CRM_Core_BAO_Cache::encode($value);