From df21d36774aee47487b882e219275b12cdd52e5e Mon Sep 17 00:00:00 2001 From: Dan Garner Date: Thu, 20 Jun 2024 08:14:59 +0100 Subject: [PATCH] XMDS: deadlocks occurring when a lot of displays make concurrent connections (#2567) * XMDS: optimise the way notifications are inserted, refactor and document alertDisplayUp (no functionality changes). Only assess operating hours if we're likely to send an email. * DisplayGroup: refactor dynamic display group assessment to use the cache instead of the DB. * Tags: ensure we set the original value to any entity we decorate with tag links. --- lib/Controller/Display.php | 1 - lib/Controller/Tag.php | 6 +- lib/Entity/Display.php | 6 +- lib/Entity/Notification.php | 38 ++++--- lib/Event/TriggerTaskEvent.php | 44 +++----- lib/Factory/TagTrait.php | 16 ++- lib/Listener/DisplayGroupListener.php | 10 +- lib/Listener/TaskListener.php | 27 ++--- lib/Middleware/ListenersMiddleware.php | 3 +- lib/XTR/MaintenanceRegularTask.php | 14 ++- lib/Xmds/Soap.php | 150 +++++++++++++++---------- 11 files changed, 185 insertions(+), 130 deletions(-) diff --git a/lib/Controller/Display.php b/lib/Controller/Display.php index d447a7863e..3ba8d1ec52 100644 --- a/lib/Controller/Display.php +++ b/lib/Controller/Display.php @@ -1867,7 +1867,6 @@ function edit(Request $request, Response $response, $id) // Tags are stored on the displaygroup, we're just passing through here if ($this->getUser()->featureEnabled('tag.tagging')) { - $display->setOriginalValue('tags', $display->tags); if (is_array($sanitizedParams->getParam('tags'))) { $tags = $this->tagFactory->tagsFromJson($sanitizedParams->getArray('tags')); } else { diff --git a/lib/Controller/Tag.php b/lib/Controller/Tag.php index f4fb47a657..6332e27dd2 100644 --- a/lib/Controller/Tag.php +++ b/lib/Controller/Tag.php @@ -27,6 +27,7 @@ use Xibo\Event\DisplayGroupLoadEvent; use Xibo\Event\TagDeleteEvent; use Xibo\Event\TagEditEvent; +use Xibo\Event\TriggerTaskEvent; use Xibo\Factory\CampaignFactory; use Xibo\Factory\DisplayFactory; use Xibo\Factory\DisplayGroupFactory; @@ -743,7 +744,10 @@ public function editMultiple(Request $request, Response $response) // Once we're done, and if we're a Display entity, we need to calculate the dynamic display groups if ($targetType === 'display') { // Background update. - $this->getConfig()->changeSetting('DYNAMIC_DISPLAY_GROUP_ASSESS', 1); + $this->getDispatcher()->dispatch( + new TriggerTaskEvent('\Xibo\XTR\MaintenanceRegularTask', 'DYNAMIC_DISPLAY_GROUP_ASSESSED'), + TriggerTaskEvent::$NAME + ); } } else { $this->getLog()->debug('Tags were not changed'); diff --git a/lib/Entity/Display.php b/lib/Entity/Display.php index d4a09ebd2e..8c04c7abc0 100644 --- a/lib/Entity/Display.php +++ b/lib/Entity/Display.php @@ -25,6 +25,7 @@ use Respect\Validation\Validator as v; use Stash\Interfaces\PoolInterface; use Xibo\Event\DisplayGroupLoadEvent; +use Xibo\Event\TriggerTaskEvent; use Xibo\Factory\DisplayFactory; use Xibo\Factory\DisplayGroupFactory; use Xibo\Factory\DisplayProfileFactory; @@ -884,7 +885,10 @@ public function save($options = []) // Trigger an update of all dynamic DisplayGroups? if ($this->hasPropertyChanged('display') || $this->hasPropertyChanged('tags')) { // Background update. - $this->config->changeSetting('DYNAMIC_DISPLAY_GROUP_ASSESS', 1); + $this->getDispatcher()->dispatch( + new TriggerTaskEvent('\Xibo\XTR\MaintenanceRegularTask', 'DYNAMIC_DISPLAY_GROUP_ASSESSED'), + TriggerTaskEvent::$NAME + ); } } diff --git a/lib/Entity/Notification.php b/lib/Entity/Notification.php index 4dd3961848..b142e36eef 100644 --- a/lib/Entity/Notification.php +++ b/lib/Entity/Notification.php @@ -257,16 +257,19 @@ public function load($options = []) * Save Notification * @throws InvalidArgumentException */ - public function save() + public function save(): void { $this->validate(); - if ($this->notificationId == null) + $isNewRecord = false; + if ($this->notificationId == null) { + $isNewRecord = true; $this->add(); - else + } else { $this->edit(); + } - $this->manageAssignments(); + $this->manageAssignments($isNewRecord); } /** @@ -346,13 +349,20 @@ private function edit() /** * Manage assignements in DB */ - private function manageAssignments() + private function manageAssignments(bool $isNewRecord): void { $this->linkUserGroups(); - $this->unlinkUserGroups(); + + // Only unlink if we're not new (otherwise there is no point as we can't have any links yet) + if (!$isNewRecord) { + $this->unlinkUserGroups(); + } $this->linkDisplayGroups(); - $this->unlinkDisplayGroups(); + + if (!$isNewRecord) { + $this->unlinkDisplayGroups(); + } $this->manageRealisedUserLinks(); } @@ -360,10 +370,11 @@ private function manageAssignments() /** * Manage the links in the User notification table */ - private function manageRealisedUserLinks() + private function manageRealisedUserLinks(bool $isNewRecord = false): void { - // Delete links that no longer exist - $this->getStore()->update(' + if (!$isNewRecord) { + // Delete links that no longer exist + $this->getStore()->update(' DELETE FROM `lknotificationuser` WHERE `notificationId` = :notificationId AND `userId` NOT IN ( SELECT `userId` @@ -373,9 +384,10 @@ private function manageRealisedUserLinks() WHERE `lknotificationgroup`.notificationId = :notificationId2 ) AND userId <> 0 ', [ - 'notificationId' => $this->notificationId, - 'notificationId2' => $this->notificationId - ]); + 'notificationId' => $this->notificationId, + 'notificationId2' => $this->notificationId + ]); + } // Pop in new links following from this adjustment $this->getStore()->update(' diff --git a/lib/Event/TriggerTaskEvent.php b/lib/Event/TriggerTaskEvent.php index 444c499ff9..18d0965b32 100644 --- a/lib/Event/TriggerTaskEvent.php +++ b/lib/Event/TriggerTaskEvent.php @@ -22,33 +22,26 @@ namespace Xibo\Event; +/** + * An event which triggers the provided task to Run Now (at the next XTR poll) + * optionally clears a cache key to provide further instructions to the task that's running + */ class TriggerTaskEvent extends Event { - public static $NAME = 'trigger.task.event'; - /** - * @var string - */ - private $className; - /** - * @var string - */ - private $setting; - /** - * @var mixed|null - */ - private $settingValue; + public static string $NAME = 'trigger.task.event'; /** - * @param string $className + * @param string $className Class name of the task to be run + * @param string $key Cache Key to be dropped */ - public function __construct(string $className, string $setting = '', $value = null) - { - $this->className = $className; - $this->setting = $setting; - $this->settingValue = $value; + public function __construct( + private readonly string $className, + private readonly string $key = '' + ) { } /** + * Returns the class name for the task to be run * @return string */ public function getClassName(): string @@ -56,13 +49,12 @@ public function getClassName(): string return $this->className; } - public function getSetting(): string - { - return $this->setting; - } - - public function getSettingValue() + /** + * Returns the cache key to be dropped + * @return string + */ + public function getKey(): string { - return $this->settingValue; + return $this->key; } } diff --git a/lib/Factory/TagTrait.php b/lib/Factory/TagTrait.php index 6110886fdc..11a4aefd0c 100644 --- a/lib/Factory/TagTrait.php +++ b/lib/Factory/TagTrait.php @@ -58,13 +58,18 @@ public function loadTagsByEntityId(string $table, string $column, int $entityId) * @param string $table * @param string $column * @param array $entityIds - * @param array $entries + * @param \Xibo\Entity\EntityTrait[] $entries */ - public function decorateWithTagLinks(string $table, string $column, array $entityIds, array $entries) + public function decorateWithTagLinks(string $table, string $column, array $entityIds, array $entries): void { - $sql = 'SELECT tag.tagId, tag.tag, `'. $table .'`.value, `'.$table.'`.'.$column.' FROM `tag` INNER JOIN `'.$table.'` ON `'.$table.'`.tagId = tag.tagId WHERE `'.$table.'`.'.$column.' IN('. implode(',', $entityIds).')'; + // Query to get all tags from a tag link table for a set of entityIds + $sql = 'SELECT `tag`.`tagId`, `tag`.`tag`, `' . $table . '`.`value`, `' . $table . '`.`' . $column . '`' + . ' FROM `tag` ' + . ' INNER JOIN `' . $table . '` ON `' . $table . '`.`tagId` = `tag`.`tagId` ' + . ' WHERE `' . $table . '`.`' . $column . '` IN(' . implode(',', $entityIds) .')'; foreach ($this->getStore()->select($sql, []) as $row) { + // Add each tag returned above to its respective entity $sanitizedRow = $this->getSanitizer($row); $tagLink = new TagLink($this->getStore(), $this->getLog(), $this->getDispatcher()); @@ -78,6 +83,11 @@ public function decorateWithTagLinks(string $table, string $column, array $entit } } } + + // Set the original value on the entity. + foreach ($entries as $entry) { + $entry->setOriginalValue('tags', $entry->tags); + } } public function getTagUsageByEntity(string $tagLinkTable, string $idColumn, string $nameColumn, string $entity, int $tagId, &$entries) diff --git a/lib/Listener/DisplayGroupListener.php b/lib/Listener/DisplayGroupListener.php index 6e6af43c52..5eacb3d96f 100644 --- a/lib/Listener/DisplayGroupListener.php +++ b/lib/Listener/DisplayGroupListener.php @@ -197,12 +197,14 @@ public function onFolderMoving(FolderMovingEvent $event) * @param EventDispatcherInterface $dispatcher * @return void */ - public function onTagDelete(TagDeleteEvent $event, $eventName, EventDispatcherInterface $dispatcher) + public function onTagDelete(TagDeleteEvent $event, $eventName, EventDispatcherInterface $dispatcher): void { $displays = $this->storageService->select(' SELECT lktagdisplaygroup.displayGroupId - FROM `lktagdisplaygroup` INNER JOIN `displaygroup` - ON `lktagdisplaygroup`.displayGroupId = `displaygroup`.displayGroupId AND `displaygroup`.isDisplaySpecific = 1 + FROM `lktagdisplaygroup` + INNER JOIN `displaygroup` + ON `lktagdisplaygroup`.displayGroupId = `displaygroup`.displayGroupId + AND `displaygroup`.isDisplaySpecific = 1 WHERE `lktagdisplaygroup`.tagId = :tagId', [ 'tagId' => $event->getTagId() ]); @@ -214,7 +216,7 @@ public function onTagDelete(TagDeleteEvent $event, $eventName, EventDispatcherIn if (count($displays) > 0) { $dispatcher->dispatch( - new TriggerTaskEvent('\Xibo\XTR\MaintenanceRegularTask', 'DYNAMIC_DISPLAY_GROUP_ASSESS', 1), + new TriggerTaskEvent('\Xibo\XTR\MaintenanceRegularTask', 'DYNAMIC_DISPLAY_GROUP_ASSESSED'), TriggerTaskEvent::$NAME ); } diff --git a/lib/Listener/TaskListener.php b/lib/Listener/TaskListener.php index 5e901dcf85..b4eefadda0 100644 --- a/lib/Listener/TaskListener.php +++ b/lib/Listener/TaskListener.php @@ -22,33 +22,24 @@ namespace Xibo\Listener; +use Stash\Interfaces\PoolInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Xibo\Event\TriggerTaskEvent; use Xibo\Factory\TaskFactory; use Xibo\Service\ConfigServiceInterface; /** - * Task events + * A listener for events related to tasks */ class TaskListener { use ListenerLoggerTrait; - /** - * @var TaskFactory - */ - private $taskFactory; - /** - * @var ConfigServiceInterface - */ - private $configService; - public function __construct( - TaskFactory $taskFactory, - ConfigServiceInterface $configService + private readonly TaskFactory $taskFactory, + private readonly ConfigServiceInterface $configService, + private readonly PoolInterface $pool ) { - $this->taskFactory = $taskFactory; - $this->configService = $configService; } /** @@ -68,12 +59,14 @@ public function registerWithDispatcher(EventDispatcherInterface $dispatcher) : T * @throws \Xibo\Support\Exception\InvalidArgumentException * @throws \Xibo\Support\Exception\NotFoundException */ - public function onTriggerTask(TriggerTaskEvent $event) + public function onTriggerTask(TriggerTaskEvent $event): void { - if (!empty($event->getSetting()) && !empty($event->getSettingValue())) { - $this->configService->changeSetting($event->getSetting(), $event->getSettingValue()); + if (!empty($event->getKey())) { + // Drop this setting from the cache + $this->pool->deleteItem($event->getKey()); } + // Mark the task to run now $task = $this->taskFactory->getByClass($event->getClassName()); $task->runNow = 1; $task->save(); diff --git a/lib/Middleware/ListenersMiddleware.php b/lib/Middleware/ListenersMiddleware.php index a33ac95d8a..8aaf677d5a 100644 --- a/lib/Middleware/ListenersMiddleware.php +++ b/lib/Middleware/ListenersMiddleware.php @@ -146,7 +146,8 @@ public static function setListeners(App $app) // Listen for event that affect Task (new TaskListener( $c->get('taskFactory'), - $c->get('configService') + $c->get('configService'), + $c->get('pool') )) ->useLogger($c->get('logger')) ->registerWithDispatcher($dispatcher); diff --git a/lib/XTR/MaintenanceRegularTask.php b/lib/XTR/MaintenanceRegularTask.php index 66a19f5660..511e5e1823 100644 --- a/lib/XTR/MaintenanceRegularTask.php +++ b/lib/XTR/MaintenanceRegularTask.php @@ -493,15 +493,23 @@ private function publishLayouts() /** * Assess any eligible dynamic display groups if necessary * @return void + * @throws \Xibo\Support\Exception\NotFoundException */ - private function assessDynamicDisplayGroups() + private function assessDynamicDisplayGroups(): void { $this->runMessage .= '## ' . __('Assess Dynamic Display Groups') . PHP_EOL; - if ($this->config->getSetting('DYNAMIC_DISPLAY_GROUP_ASSESS', 0) == 1) { + // Do we have a cache key set to say that dynamic display group assessment has been completed? + $cache = $this->pool->getItem('DYNAMIC_DISPLAY_GROUP_ASSESSED'); + if ($cache->isMiss()) { Profiler::start('RegularMaintenance::assessDynamicDisplayGroups', $this->log); - $this->config->changeSetting('DYNAMIC_DISPLAY_GROUP_ASSESS', 0); + // Set the cache key with a long expiry and save. + $cache->set(true); + $cache->expiresAt(Carbon::now()->addYear()); + $this->pool->save($cache); + + // Process each dynamic display group $count = 0; foreach ($this->displayGroupFactory->getByIsDynamic(1) as $group) { diff --git a/lib/Xmds/Soap.php b/lib/Xmds/Soap.php index 489e4c1cfa..410d7c8711 100644 --- a/lib/Xmds/Soap.php +++ b/lib/Xmds/Soap.php @@ -2551,94 +2551,124 @@ protected function authDisplay($hardwareKey) /** * Alert Display Up - * @throws \phpmailerException + * assesses whether a notification is required to be sent for this display, and only does something if the + * display is currently marked as offline (i.e. it is coming back online again) + * this is only called in Register * @throws NotFoundException */ - protected function alertDisplayUp() + protected function alertDisplayUp(): void { $maintenanceEnabled = $this->getConfig()->getSetting('MAINTENANCE_ENABLED'); if ($this->display->loggedIn == 0) { - $this->getLog()->info(sprintf('Display %s was down, now its up.', $this->display->display)); // Log display up $this->displayEventFactory->createEmpty()->displayUp($this->display->displayId); - $dayPartId = $this->display->getSetting('dayPartId', null,['displayOverride' => true]); - - $operatingHours = true; - - if ($dayPartId !== null) { - try { - $dayPart = $this->dayPartFactory->getById($dayPartId); - - $startTimeArray = explode(':', $dayPart->startTime); - $startTime = Carbon::now()->setTime(intval($startTimeArray[0]), intval($startTimeArray[1])); - - $endTimeArray = explode(':', $dayPart->endTime); - $endTime = Carbon::now()->setTime(intval($endTimeArray[0]), intval($endTimeArray[1])); - - $now = Carbon::now(); - - // exceptions - foreach ($dayPart->exceptions as $exception) { + // Do we need to email? + if ($this->display->emailAlert == 1 + && ($maintenanceEnabled == 'On' || $maintenanceEnabled == 'Protected') + && $this->getConfig()->getSetting('MAINTENANCE_EMAIL_ALERTS') == 1 + ) { + // Only send alerts during operating hours. + if ($this->isInsideOperatingHours()) { + $subject = sprintf(__('Recovery for Display %s'), $this->display->display); + $body = sprintf( + __('Display ID %d is now back online %s'), + $this->display->displayId, + Carbon::now()->format(DateFormatHelper::getSystemFormat()) + ); - // check if we are on exception day and if so override the startTime and endTime accordingly - if ($exception['day'] == Carbon::now()->format('D')) { - $exceptionsStartTime = explode(':', $exception['start']); - $startTime = Carbon::now()->setTime(intval($exceptionsStartTime[0]), intval($exceptionsStartTime[1])); + // Create a notification assigned to system-wide user groups + try { + $notification = $this->notificationFactory->createSystemNotification( + $subject, + $body, + Carbon::now() + ); - $exceptionsEndTime = explode(':', $exception['end']); - $endTime = Carbon::now()->setTime(intval($exceptionsEndTime[0]), intval($exceptionsEndTime[1])); + // Get groups which have been configured to receive notifications + foreach ($this->userGroupFactory + ->getDisplayNotificationGroups($this->display->displayGroupId) as $group) { + $notification->assignUserGroup($group); } - } - // check if we are inside the operating hours for this display - we use that flag to decide if we need to create a notification and send an email. - if (($now >= $startTime && $now <= $endTime)) { - $operatingHours = true; - } else { - $operatingHours = false; + // Save the notification and insert the links, etc. + $notification->save(); + } catch (\Exception) { + $this->getLog()->error(sprintf( + 'Unable to send email alert for display %s with subject %s and body %s', + $this->display->display, + $subject, + $body + )); } - - } catch (NotFoundException $e) { - $this->getLog()->debug('Unknown dayPartId set on Display Profile for displayId ' . $this->display->displayId); + } else { + $this->getLog()->info('Not sending recovery email for Display - ' + . $this->display->display . ' we are outside of its operating hours'); } + } else { + $this->getLog()->debug(sprintf( + 'No email required. Email Alert: %d, Enabled: %s, Email Enabled: %s.', + $this->display->emailAlert, + $maintenanceEnabled, + $this->getConfig()->getSetting('MAINTENANCE_EMAIL_ALERTS') + )); } + } + } - // Do we need to email? - if ($this->display->emailAlert == 1 && ($maintenanceEnabled == 'On' || $maintenanceEnabled == 'Protected') - && $this->getConfig()->getSetting('MAINTENANCE_EMAIL_ALERTS') == 1) { + /** + * Is the display currently inside operating hours? + * @return bool + * @throws \Xibo\Support\Exception\NotFoundException + */ + private function isInsideOperatingHours(): bool + { + $dayPartId = $this->display->getSetting('dayPartId', null, ['displayOverride' => true]); + if ($dayPartId !== null) { + try { + $dayPart = $this->dayPartFactory->getById($dayPartId); - // for displays without dayPartId set, this is always true, otherwise we check if we are inside the operating hours set for this display - if ($operatingHours) { - $subject = sprintf(__("Recovery for Display %s"), $this->display->display); - $body = sprintf(__("Display ID %d is now back online %s"), $this->display->displayId, - Carbon::now()->format(DateFormatHelper::getSystemFormat())); + $startTimeArray = explode(':', $dayPart->startTime); + $startTime = Carbon::now()->setTime(intval($startTimeArray[0]), intval($startTimeArray[1])); - // Create a notification assigned to system wide user groups - try { - $notification = $this->notificationFactory->createSystemNotification($subject, $body, - Carbon::now()); + $endTimeArray = explode(':', $dayPart->endTime); + $endTime = Carbon::now()->setTime(intval($endTimeArray[0]), intval($endTimeArray[1])); - // Add in any displayNotificationGroups, with permissions - foreach ($this->userGroupFactory->getDisplayNotificationGroups($this->display->displayGroupId) as $group) { - $notification->assignUserGroup($group); - } + $now = Carbon::now(); - $notification->save(); + // handle exceptions + foreach ($dayPart->exceptions as $exception) { + // check if we are on exception day and if so override the startTime and endTime accordingly + if ($exception['day'] == Carbon::now()->format('D')) { + // Parse the start/end times into the current day. + $exceptionsStartTime = explode(':', $exception['start']); + $startTime = Carbon::now()->setTime( + intval($exceptionsStartTime[0]), + intval($exceptionsStartTime[1]) + ); - } catch (\Exception $e) { - $this->getLog()->error(sprintf('Unable to send email alert for display %s with subject %s and body %s', - $this->display->display, $subject, $body)); + $exceptionsEndTime = explode(':', $exception['end']); + $endTime = Carbon::now()->setTime( + intval($exceptionsEndTime[0]), + intval($exceptionsEndTime[1]) + ); } - } else { - $this->getLog()->info('Not sending recovery email for Display - ' . $this->display->display . ' we are outside of its operating hours'); } - } else { - $this->getLog()->debug(sprintf('No email required. Email Alert: %d, Enabled: %s, Email Enabled: %s.', $this->display->emailAlert, $maintenanceEnabled, $this->getConfig()->getSetting('MAINTENANCE_EMAIL_ALERTS'))); + + // check if we are inside the operating hours for this display - we use that flag to decide + // if we need to create a notification and send an email. + if (($now >= $startTime && $now <= $endTime)) { + return true; + } + } catch (NotFoundException) { + $this->getLog()->debug('Unknown dayPartId set on Display Profile for displayId ' + . $this->display->displayId); } } + return false; } /**