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.

Sign up for GitHub

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

Jump to bottom

(dev/core#174) Full PSR-16 compliance for SqlGroup #12379

Merged
merged 2 commits into from
Jul 2, 2018
Merged

(dev/core#174) Full PSR-16 compliance for SqlGroup #12379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be5eea0
(dev/core#174) SqlGroup - Rewrite with PSR-16 and TTL support. Bypass…
totten Jun 21, 2018
fd33fea
(dev/core#174) CRM_Core_BAO_Cache - Periodically flush expired records
totten Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions CRM/Core/BAO/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ public static function restoreSessionFromCache($names) {
* @param bool $table
* @param bool $prevNext
*/
public static function cleanup($session = FALSE, $table = FALSE, $prevNext = FALSE) {
public static function cleanup($session = FALSE, $table = FALSE, $prevNext = FALSE, $expired = FALSE) {
// first delete all sessions more than 20 minutes old which are related to any potential transaction
$timeIntervalMins = (int) Civi::settings()->get('secure_cache_timeout_minutes');
if ($timeIntervalMins && $session) {
Expand Down Expand Up @@ -338,10 +338,10 @@ public static function cleanup($session = FALSE, $table = FALSE, $prevNext = FAL
$timeIntervalDays = 2;

if (mt_rand(1, 100000) % $cleanUpNumber == 0) {
$session = $table = $prevNext = TRUE;
$expired = $session = $table = $prevNext = TRUE;
}

if (!$session && !$table && !$prevNext) {
if (!$session && !$table && !$prevNext && !$expired) {
return;
}

Expand All @@ -363,6 +363,14 @@ public static function cleanup($session = FALSE, $table = FALSE, $prevNext = FAL
";
CRM_Core_DAO::executeQuery($sql);
}

if ($expired) {
$sql = "DELETE FROM civicrm_cache WHERE expired_date < %1";
$params = [
1 => [date(CRM_Utils_Cache_SqlGroup::TS_FMT, CRM_Utils_Time::getTimeRaw()), 'String'],
];
CRM_Core_DAO::executeQuery($sql, $params);
}
}

/**
Expand Down
130 changes: 107 additions & 23 deletions CRM/Utils/Cache/SqlGroup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,11 @@
*/
class CRM_Utils_Cache_SqlGroup implements CRM_Utils_Cache_Interface {

// 6*60*60
const DEFAULT_TTL = 21600;

const TS_FMT = 'Y-m-d H:i:s';
use CRM_Utils_Cache_NaiveMultipleTrait; // TODO Consider native implementation.
use CRM_Utils_Cache_NaiveHasTrait; // TODO Native implementation

/**
* The host name of the memcached server.
Expand All @@ -56,7 +59,18 @@ class CRM_Utils_Cache_SqlGroup implements CRM_Utils_Cache_Interface {
/**
* @var array in-memory cache to optimize redundant get()s
*/
protected $frontCache;
protected $valueCache;

/**
* @var array in-memory cache to optimize redundant get()s
* Note: expiresCache[$key]===NULL means cache-miss
*/
protected $expiresCache;

/**
* @var string
*/
protected $table;

/**
* Constructor.
Expand All @@ -71,6 +85,7 @@ class CRM_Utils_Cache_SqlGroup implements CRM_Utils_Cache_Interface {
* @return \CRM_Utils_Cache_SqlGroup
*/
public function __construct($config) {
$this->table = CRM_Core_DAO_Cache::getTableName();
if (isset($config['group'])) {
$this->group = $config['group'];
}
Expand All @@ -83,7 +98,7 @@ public function __construct($config) {
else {
$this->componentID = NULL;
}
$this->frontCache = array();
$this->valueCache = array();
if (CRM_Utils_Array::value('prefetch', $config, TRUE)) {
$this->prefetch();
}
Expand All @@ -96,11 +111,47 @@ public function __construct($config) {
* @return bool
*/
public function set($key, $value, $ttl = NULL) {
if ($ttl !== NULL) {
throw new \RuntimeException("FIXME: " . __CLASS__ . "::set() should support non-NULL TTL");
CRM_Utils_Cache::assertValidKey($key);

$lock = Civi::lockManager()->acquire("cache.{$this->group}_{$key}._null");
if (!$lock->isAcquired()) {
throw new \CRM_Utils_Cache_CacheException("SqlGroup: Failed to acquire lock on cache key.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these largely ignored? Failure to aquire a lock should not be a fatal error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this behavior is reproducing the behavior of CRM_Core_BAO_Cache::setItem(). The only change (based on feedback from Seamus) was replacing CRM_Core_Error::fatal(...) with throw new Exception(...).

IIRC, what you're referring to about the hard-failure for CiviMail is addressed within the locking subsystem (ie within CRM_Core_Lock). That has some hacks which say, "If we're doing a CiviMail cronjob, then we won't try to acquire more locks, and we'll tell callers that our locks are working just fine." From the perspective of a specific lock user, they should still request/check locks as normal. The fudgery (as I understood it) was encapsulated within CRM_Core_Lock.

This code should behave just as well (or badly) as the previous code from CRM_Core_BAO_Cache.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason why i suggested Exception instead of fatal is that i believed that fatal calls were being removed in favor of exceptions.

}

$dataExists = CRM_Core_DAO::singleValueQuery("SELECT COUNT(*) FROM {$this->table} WHERE {$this->where($key)}");
$expires = CRM_Utils_Date::convertCacheTtlToExpires($ttl, self::DEFAULT_TTL);

$dataSerialized = CRM_Core_BAO_Cache::encode($value);

// This table has a wonky index, so we cannot use REPLACE or
// "INSERT ... ON DUPE". Instead, use SELECT+(INSERT|UPDATE).
if ($dataExists) {
$sql = "UPDATE {$this->table} SET data = %1, created_date = FROM_UNIXTIME(%2), expired_date = FROM_UNIXTIME(%3) WHERE {$this->where($key)}";
$args = array(
1 => array($dataSerialized, 'String'),
2 => array(time(), 'Positive'),
3 => array($expires, 'Positive'),
);
$dao = CRM_Core_DAO::executeQuery($sql, $args, FALSE, NULL, FALSE, FALSE);
}
else {
$sql = "INSERT INTO {$this->table} (group_name,path,data,created_date,expired_date) VALUES (%1,%2,%3,FROM_UNIXTIME(%4),FROM_UNIXTIME(%5))";
$args = array(
1 => [$this->group, 'String'],
2 => [$key, 'String'],
3 => [$dataSerialized, 'String'],
4 => [time(), 'Positive'],
5 => [$expires, 'Positive'],
);
$dao = CRM_Core_DAO::executeQuery($sql, $args, FALSE, NULL, FALSE, FALSE);
}
CRM_Core_BAO_Cache::setItem($value, $this->group, $key, $this->componentID);
$this->frontCache[$key] = $value;

$lock->release();

$dao->free();

$this->valueCache[$key] = CRM_Core_BAO_Cache::decode($dataSerialized);
$this->expiresCache[$key] = $expires;
return TRUE;
}

Expand All @@ -111,13 +162,21 @@ public function set($key, $value, $ttl = NULL) {
* @return mixed
*/
public function get($key, $default = NULL) {
if ($default !== NULL) {
throw new \RuntimeException("FIXME: " . __CLASS__ . "::get() only supports NULL default");
}
if (!array_key_exists($key, $this->frontCache)) {
$this->frontCache[$key] = CRM_Core_BAO_Cache::getItem($this->group, $key, $this->componentID);
CRM_Utils_Cache::assertValidKey($key);
if (!isset($this->expiresCache[$key]) || time() >= $this->expiresCache[$key]) {
$sql = "SELECT path, data, UNIX_TIMESTAMP(expired_date) as expires FROM {$this->table} WHERE " . $this->where($key);
$dao = CRM_Core_DAO::executeQuery($sql);
while ($dao->fetch()) {
$this->expiresCache[$key] = $dao->expires;
$this->valueCache[$key] = CRM_Core_BAO_Cache::decode($dao->data);
}
$dao->free();
}
return $this->frontCache[$key];
return (isset($this->expiresCache[$key]) && time() < $this->expiresCache[$key]) ? $this->reobjectify($this->valueCache[$key]) : $default;
}

private function reobjectify($value) {
return is_object($value) ? unserialize(serialize($value)) : $value;
}

/**
Expand All @@ -127,26 +186,35 @@ public function get($key, $default = NULL) {
* @return mixed
*/
public function getFromFrontCache($key, $default = NULL) {
return CRM_Utils_Array::value($key, $this->frontCache, $default);
if (isset($this->expiresCache[$key]) && time() < $this->expiresCache[$key] && $this->valueCache[$key]) {
return $this->reobjectify($this->valueCache[$key]);
}
else {
return $default;
}
}

public function has($key) {
$this->get($key);
return isset($this->expiresCache[$key]) && time() < $this->expiresCache[$key];
}

/**
* @param string $key
* @return bool
*/
public function delete($key) {
CRM_Core_BAO_Cache::deleteGroup($this->group, $key, FALSE);
CRM_Core_BAO_Cache::$_cache = NULL; // FIXME: remove multitier cache
CRM_Utils_Cache::singleton()->flush(); // FIXME: remove multitier cache
unset($this->frontCache[$key]);
CRM_Utils_Cache::assertValidKey($key);
CRM_Core_DAO::executeQuery("DELETE FROM {$this->table} WHERE {$this->where($key)}");
unset($this->valueCache[$key]);
unset($this->expiresCache[$key]);
return TRUE;
}

public function flush() {
CRM_Core_BAO_Cache::deleteGroup($this->group, NULL, FALSE);
CRM_Core_BAO_Cache::$_cache = NULL; // FIXME: remove multitier cache
CRM_Utils_Cache::singleton()->flush(); // FIXME: remove multitier cache
$this->frontCache = array();
CRM_Core_DAO::executeQuery("DELETE FROM {$this->table} WHERE {$this->where()}");
$this->valueCache = array();
$this->expiresCache = array();
return TRUE;
}

Expand All @@ -155,7 +223,23 @@ public function clear() {
}

public function prefetch() {
$this->frontCache = CRM_Core_BAO_Cache::getItems($this->group, $this->componentID);
$dao = CRM_Core_DAO::executeQuery("SELECT path, data, UNIX_TIMESTAMP(expired_date) AS expires FROM {$this->table} WHERE " . $this->where(NULL));
$this->valueCache = array();
$this->expiresCache = array();
while ($dao->fetch()) {
$this->valueCache[$dao->path] = CRM_Core_BAO_Cache::decode($dao->data);
$this->expiresCache[$dao->path] = $dao->expires;
}
$dao->free();
}

protected function where($path = NULL) {
$clauses = array();
$clauses[] = ('group_name = "' . CRM_Core_DAO::escapeString($this->group) . '"');
if ($path) {
$clauses[] = ('path = "' . CRM_Core_DAO::escapeString($path) . '"');
}
return $clauses ? implode(' AND ', $clauses) : '(1)';
}

}
5 changes: 3 additions & 2 deletions api/v3/Job.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,14 +619,15 @@ function civicrm_api3_job_cleanup($params) {
$session = CRM_Utils_Array::value('session', $params, TRUE);
$tempTable = CRM_Utils_Array::value('tempTables', $params, TRUE);
$jobLog = CRM_Utils_Array::value('jobLog', $params, TRUE);
$expired = CRM_Utils_Array::value('expiredDbCache', $params, TRUE);
$prevNext = CRM_Utils_Array::value('prevNext', $params, TRUE);
$dbCache = CRM_Utils_Array::value('dbCache', $params, FALSE);
$memCache = CRM_Utils_Array::value('memCache', $params, FALSE);
$tplCache = CRM_Utils_Array::value('tplCache', $params, FALSE);
$wordRplc = CRM_Utils_Array::value('wordRplc', $params, FALSE);

if ($session || $tempTable || $prevNext) {
CRM_Core_BAO_Cache::cleanup($session, $tempTable, $prevNext);
if ($session || $tempTable || $prevNext || $expired) {
CRM_Core_BAO_Cache::cleanup($session, $tempTable, $prevNext, $expired);
}

if ($jobLog) {
Expand Down
4 changes: 3 additions & 1 deletion tests/phpunit/CRM/Utils/Cache/SqlGroupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ public function testTwoInstance() {
));
$fooValue = array('whiz' => 'bang', 'bar' => 3);
$a->set('foo', $fooValue);
$this->assertEquals($a->get('foo'), array('whiz' => 'bang', 'bar' => 3));
$getValue = $a->get('foo');
$expectValue = array('whiz' => 'bang', 'bar' => 3);
$this->assertEquals($getValue, $expectValue);

$b = new CRM_Utils_Cache_SqlGroup(array(
'group' => 'testTwoInstance',
Expand Down
41 changes: 41 additions & 0 deletions tests/phpunit/E2E/Cache/SqlGroupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php
/*
+--------------------------------------------------------------------+
| CiviCRM version 5 |
+--------------------------------------------------------------------+
| Copyright CiviCRM LLC (c) 2004-2018 |
+--------------------------------------------------------------------+
| This file is a part of CiviCRM. |
| |
| CiviCRM is free software; you can copy, modify, and distribute it |
| under the terms of the GNU Affero General Public License |
| Version 3, 19 November 2007 and the CiviCRM Licensing Exception. |
| |
| CiviCRM is distributed in the hope that it will be useful, but |
| WITHOUT ANY WARRANTY; without even the implied warranty of |
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. |
| See the GNU Affero General Public License for more details. |
| |
| You should have received a copy of the GNU Affero General Public |
| License along with this program; if not, contact CiviCRM LLC |
| at info[AT]civicrm[DOT]org. If you have questions about the |
| GNU Affero General Public License or the licensing of CiviCRM, |
| see the CiviCRM license FAQ at http://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/

/**
* Verify that CRM_Utils_Cache_SqlGroup complies with PSR-16.
*
* @group e2e
*/
class E2E_Cache_SqlGroupTest extends E2E_Cache_CacheTestCase {

public function createSimpleCache() {
return CRM_Utils_Cache::create([
'name' => 'e2e sqlgroup test',
'type' => ['SqlGroup'],
]);
}

}