XMDS: deadlocks occurring when a lot of displays make concurrent conn…

…ections (#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.
xibosignage · Jun 20, 2024 · df21d36 · df21d36
1 parent 4c312c1
commit df21d36
Show file tree

Hide file tree

Showing 11 changed files with 185 additions and 130 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,24 +349,32 @@ 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();
     }
 
     /**
      * 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
@@ -22,47 +22,39 @@
 
 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
     {
         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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {