diff --git a/core/Archive/ArchiveInvalidator.php b/core/Archive/ArchiveInvalidator.php index 14140046aaf..59370cf0f13 100644 --- a/core/Archive/ArchiveInvalidator.php +++ b/core/Archive/ArchiveInvalidator.php @@ -60,15 +60,26 @@ public function __construct(Model $model) public function rememberToInvalidateArchivedReportsLater($idSite, Date $date) { - $key = $this->buildRememberArchivedReportId($idSite, $date->toString()); - $value = Option::get($key); + // To support multiple transactions at once, look for any other process to have set (and committed) + // this report to be invalidated. + $key = $this->buildRememberArchivedReportIdForSiteAndDate($idSite, $date->toString()); // we do not really have to get the value first. we could simply always try to call set() and it would update or // insert the record if needed but we do not want to lock the table (especially since there are still some // MyISAM installations) - - if (false === $value) { - Option::set($key, '1'); + $value = Option::getLike($key . '%'); + + // In order to support multiple concurrent transactions, add our pid to the end of the key so that it will just insert + // rather than waiting on some other process to commit before proceeding.The issue is that with out this, more than + // one process is trying to add the exact same value to the table, which causes contention. With the pid suffixed to + // the value, each process can successfully enter its own row in the table. The net result will be the same. We could + // always just set this, but it would result in a lot of rows in the options table.. more than needed. With this + // change you'll have at most N rows per date/site, where N is the number of parallel requests on this same idsite/date + // that happen to run in overlapping transactions. + $mykey = $this->buildRememberArchivedReportIdProcessSafe($idSite, $date->toString()); + // getLike() returns an empty array rather than 'false' + if (empty($value)) { + Option::set($mykey, '1'); } } @@ -94,7 +105,12 @@ public function getRememberedArchivedReportsThatShouldBeInvalidated() return $sitesPerDay; } - private function buildRememberArchivedReportId($idSite, $date) + private function buildRememberArchivedReportIdForSite($idSite) + { + return $this->rememberArchivedReportIdStart . (int) $idSite; + } + + private function buildRememberArchivedReportIdForSiteAndDate($idSite, $date) { $id = $this->buildRememberArchivedReportIdForSite($idSite); $id .= '_' . trim($date); @@ -102,9 +118,12 @@ private function buildRememberArchivedReportId($idSite, $date) return $id; } - private function buildRememberArchivedReportIdForSite($idSite) + // This version is multi process safe on the insert of a new date to invalidate. + private function buildRememberArchivedReportIdProcessSafe($idSite, $date) { - return $this->rememberArchivedReportIdStart . (int) $idSite; + $id = $this->buildRememberArchivedReportIdForSiteAndDate($idSite, $date); + $id .= '_' . getmypid(); + return $id; } public function forgetRememberedArchivedReportsToInvalidateForSite($idSite) @@ -118,9 +137,11 @@ public function forgetRememberedArchivedReportsToInvalidateForSite($idSite) */ public function forgetRememberedArchivedReportsToInvalidate($idSite, Date $date) { - $id = $this->buildRememberArchivedReportId($idSite, $date->toString()); + $id = $this->buildRememberArchivedReportIdForSiteAndDate($idSite, $date->toString()); - Option::delete($id); + // The process pid is added to the end of the entry in order to support multiple concurrent transactions. + // So this must be a deleteLike call to get all the entries, where there used to only be one. + Option::deleteLike($id . '%'); } /** diff --git a/core/CronArchive.php b/core/CronArchive.php index e36ec817645..253a6a00771 100644 --- a/core/CronArchive.php +++ b/core/CronArchive.php @@ -1151,7 +1151,8 @@ public function invalidateArchivedReportsForSitesThatNeedToBeArchivedAgain() $sitesPerDays = $this->invalidator->getRememberedArchivedReportsThatShouldBeInvalidated(); foreach ($sitesPerDays as $date => $siteIds) { - $listSiteIds = implode(',', $siteIds); + //Concurrent transaction logic will end up with duplicates set. Adding array_unique to the siteIds. + $listSiteIds = implode(',', array_unique($siteIds )); try { $this->logger->info('- Will invalidate archived reports for ' . $date . ' for following websites ids: ' . $listSiteIds); diff --git a/tests/PHPUnit/Integration/DataAccess/ArchiveInvalidatorTest.php b/tests/PHPUnit/Integration/DataAccess/ArchiveInvalidatorTest.php index 27433058321..8d09c60a79f 100644 --- a/tests/PHPUnit/Integration/DataAccess/ArchiveInvalidatorTest.php +++ b/tests/PHPUnit/Integration/DataAccess/ArchiveInvalidatorTest.php @@ -66,7 +66,8 @@ public function setUp() public function test_rememberToInvalidateArchivedReportsLater_shouldCreateAnEntryInCaseThereIsNoneYet() { - $key = 'report_to_invalidate_2_2014-04-05'; + //Updated for change to allow for multiple transactions to invalidate the same report without deadlock. + $key = 'report_to_invalidate_2_2014-04-05' . '_' . getmypid(); $this->assertFalse(Option::get($key)); $this->rememberReport(2, '2014-04-05');