Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(dev/core#174) CRM_Utils_Cache_SqlGroup - Refine trivial TTL handling to stabilize tests #12427

Merged
merged 1 commit into from
Jul 6, 2018

Conversation

totten
Copy link
Member

@totten totten commented Jul 6, 2018

Overview

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

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

This patch 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
$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.

… 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.
@civibot
Copy link

civibot bot commented Jul 6, 2018

(Standard links)

@eileenmcnaughton
Copy link
Contributor

This seems fine - merging

@eileenmcnaughton eileenmcnaughton merged commit edf0e41 into civicrm:master Jul 6, 2018
@totten totten deleted the 5.4-sqlgroup-ttl-1 branch July 6, 2018 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants