From f4b01d9fa8adee1825abcc1c3a32268dc98c4856 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 24 May 2023 10:16:48 +0200 Subject: [PATCH] perf(imap): Reduce number of STATUS commands for background mailbox sync Signed-off-by: Christoph Wurst --- lib/Db/Mailbox.php | 2 +- lib/Folder.php | 8 +-- lib/IMAP/FolderMapper.php | 42 ++++++-------- lib/IMAP/MailboxStats.php | 9 +-- lib/IMAP/MailboxSync.php | 87 ++++++++++++++++++++++------ tests/Unit/IMAP/FolderMapperTest.php | 78 ++++--------------------- tests/Unit/IMAP/MailboxSyncTest.php | 31 +++++++++- 7 files changed, 135 insertions(+), 122 deletions(-) diff --git a/lib/Db/Mailbox.php b/lib/Db/Mailbox.php index 6bcfe3011c..bf3c24cf13 100644 --- a/lib/Db/Mailbox.php +++ b/lib/Db/Mailbox.php @@ -151,7 +151,7 @@ public function hasLocks(int $now): bool { * @return MailboxStats */ public function getStats(): MailboxStats { - return new MailboxStats($this->getMessages(), $this->getUnseen(), $this->getMyAcls()); + return new MailboxStats($this->getMessages(), $this->getUnseen()); } #[ReturnTypeWillChange] diff --git a/lib/Folder.php b/lib/Folder.php index 9249cd84ea..2b23c23cb4 100644 --- a/lib/Folder.php +++ b/lib/Folder.php @@ -38,7 +38,7 @@ class Folder { /** @var string */ private $delimiter; - /** @var array */ + /** @var null|array */ private $status; /** @var string[] */ @@ -50,7 +50,7 @@ public function __construct(int $accountId, Horde_Imap_Client_Mailbox $mailbox, array $attributes, ?string $delimiter, - array $status) { + ?array $status) { $this->accountId = $accountId; $this->mailbox = $mailbox; $this->attributes = $attributes; @@ -76,9 +76,9 @@ public function getAttributes(): array { } /** - * @return array + * @return null|array */ - public function getStatus(): array { + public function getStatus(): ?array { return $this->status; } diff --git a/lib/IMAP/FolderMapper.php b/lib/IMAP/FolderMapper.php index 73bda56b3e..02a2fbaac4 100644 --- a/lib/IMAP/FolderMapper.php +++ b/lib/IMAP/FolderMapper.php @@ -59,28 +59,25 @@ public function getFolders(Account $account, Horde_Imap_Client_Socket $client, 'delimiter' => true, 'attributes' => true, 'special_use' => true, - 'status' => Horde_Imap_Client::STATUS_ALL, ]); - - return array_filter(array_map(static function (array $mailbox) use ($account) { + $toPersist = array_filter($mailboxes, function (array $mailbox) { $attributes = array_flip(array_map('strtolower', $mailbox['attributes'])); if (isset($attributes['\\nonexistent'])) { // Ignore mailbox that does not exist, similar to \Horde_Imap_Client::MBOX_SUBSCRIBED_EXISTS mode - return null; + return false; } - if (in_array($mailbox['mailbox']->utf8, self::DOVECOT_SIEVE_FOLDERS, true)) { - // This is a special folder that must not be shown - return null; - } - + // This is a special folder that must not be shown + return !in_array($mailbox['mailbox']->utf8, self::DOVECOT_SIEVE_FOLDERS, true); + }); + return array_map(static function (array $mailbox) use ($account) { return new Folder( $account->getId(), $mailbox['mailbox'], $mailbox['attributes'], $mailbox['delimiter'], - $mailbox['status'], + null, ); - }, $mailboxes)); + }, $toPersist); } public function createFolder(Horde_Imap_Client_Socket $client, @@ -137,21 +134,20 @@ public function fetchFolderAcls(array $folders, * * @throws Horde_Imap_Client_Exception * - * @return MailboxStats + * @return MailboxStats[] */ public function getFoldersStatusAsObject(Horde_Imap_Client_Socket $client, - string $mailbox): MailboxStats { - $status = $client->status($mailbox); - $acls = null; - if ($client->capability->query('ACL')) { - $acls = (string)$client->getMyACLRights($mailbox); - } + array $mailboxes): array { + $multiStatus = $client->status($mailboxes); - return new MailboxStats( - $status['messages'], - $status['unseen'], - $acls - ); + $statuses = []; + foreach ($multiStatus as $mailbox => $status) { + $statuses[$mailbox] = new MailboxStats( + $status['messages'], + $status['unseen'], + ); + } + return $statuses; } /** diff --git a/lib/IMAP/MailboxStats.php b/lib/IMAP/MailboxStats.php index e3e46c7e36..bd7f1b8fb2 100644 --- a/lib/IMAP/MailboxStats.php +++ b/lib/IMAP/MailboxStats.php @@ -32,12 +32,10 @@ class MailboxStats implements JsonSerializable { private int $total; private int $unread; - private ?string $myAcls; - public function __construct(int $total, int $unread, ?string $myAcls) { + public function __construct(int $total, int $unread) { $this->total = $total; $this->unread = $unread; - $this->myAcls = $myAcls; } public function getTotal(): int { @@ -48,16 +46,11 @@ public function getUnread(): int { return $this->unread; } - public function getMyAcls(): ?string { - return $this->myAcls; - } - #[ReturnTypeWillChange] public function jsonSerialize() { return [ 'total' => $this->total, 'unread' => $this->unread, - 'myAcls' => $this->myAcls, ]; } } diff --git a/lib/IMAP/MailboxSync.php b/lib/IMAP/MailboxSync.php index 9f8f304cb6..3ff6375e70 100644 --- a/lib/IMAP/MailboxSync.php +++ b/lib/IMAP/MailboxSync.php @@ -43,8 +43,11 @@ use Psr\Log\LoggerInterface; use function array_combine; use function array_map; +use function array_reduce; +use function array_slice; use function in_array; use function json_encode; +use function shuffle; use function sprintf; use function str_starts_with; @@ -103,14 +106,16 @@ public function sync(Account $account, $namespaces = $client->getNamespaces([], [ 'ob_return' => true, ]); + $personalNamespace = $this->getPersonalNamespace($namespaces); $account->getMailAccount()->setPersonalNamespace( - $this->getPersonalNamespace($namespaces) + $personalNamespace ); } catch (Horde_Imap_Client_Exception $e) { $logger->debug('Getting namespaces for account ' . $account->getId() . ' failed: ' . $e->getMessage(), [ 'exception' => $e, ]); $namespaces = null; + $personalNamespace = null; } try { @@ -125,7 +130,7 @@ public function sync(Account $account, } $this->folderMapper->detectFolderSpecialUse($folders); - $this->atomic(function () use ($account, $folders, $namespaces) { + $mailboxes = $this->atomic(function () use ($account, $folders, $namespaces) { $old = $this->mailboxMapper->findAll($account); $indexedOld = array_combine( array_map(static function (Mailbox $mb) { @@ -134,9 +139,11 @@ public function sync(Account $account, $old ); - $this->persist($account, $folders, $indexedOld, $namespaces); + return $this->persist($account, $folders, $indexedOld, $namespaces); }, $this->dbConnection); + $this->syncMailboxStatus($mailboxes, $personalNamespace, $client); + $this->dispatcher->dispatchTyped( new MailboxesSynchronizedEvent($account) ); @@ -156,7 +163,7 @@ public function sync(Account $account, public function syncStats(Account $account, Mailbox $mailbox): void { $client = $this->imapClientFactory->getClient($account); try { - $stats = $this->folderMapper->getFoldersStatusAsObject($client, $mailbox->getName()); + $stats = $this->folderMapper->getFoldersStatusAsObject($client, [$mailbox->getName()])[$mailbox->getName()]; } catch (Horde_Imap_Client_Exception $e) { $id = $mailbox->getId(); throw new ServiceException( @@ -173,19 +180,23 @@ public function syncStats(Account $account, Mailbox $mailbox): void { $this->mailboxMapper->update($mailbox); } + /** + * @return Mailbox[] + */ private function persist(Account $account, array $folders, array $existing, - ?Horde_Imap_Client_Namespace_List $namespaces): void { + ?Horde_Imap_Client_Namespace_List $namespaces): array { + $mailboxes = []; foreach ($folders as $folder) { if (isset($existing[$folder->getMailbox()])) { - $this->updateMailboxFromFolder( + $mailboxes[] = $this->updateMailboxFromFolder( $folder, $existing[$folder->getMailbox()], $namespaces, ); } else { - $this->createMailboxFromFolder( + $mailboxes[] = $this->createMailboxFromFolder( $account, $folder, $namespaces, @@ -201,44 +212,49 @@ private function persist(Account $account, $account->getMailAccount()->setLastMailboxSync($this->timeFactory->getTime()); $this->mailAccountMapper->update($account->getMailAccount()); + + return $mailboxes; } private function getPersonalNamespace(Horde_Imap_Client_Namespace_List $namespaces): ?string { foreach ($namespaces as $namespace) { /** @var Horde_Imap_Client_Data_Namespace $namespace */ if ($namespace->type === Horde_Imap_Client::NS_PERSONAL) { - return $namespace->name; + return $namespace->name !== "" ? $namespace->name : null; } - $namespace->name; } return null; } - private function updateMailboxFromFolder(Folder $folder, Mailbox $mailbox, ?Horde_Imap_Client_Namespace_List $namespaces): void { + private function updateMailboxFromFolder(Folder $folder, Mailbox $mailbox, ?Horde_Imap_Client_Namespace_List $namespaces): Mailbox { $mailbox->setAttributes(json_encode($folder->getAttributes())); $mailbox->setDelimiter($folder->getDelimiter()); - $mailbox->setMessages($folder->getStatus()['messages'] ?? 0); - $mailbox->setUnseen($folder->getStatus()['unseen'] ?? 0); + $status = $folder->getStatus(); + if ($status !== null) { + $mailbox->setMessages($status['messages'] ?? 0); + $mailbox->setUnseen($status['unseen'] ?? 0); + } $mailbox->setSelectable(!in_array('\noselect', $folder->getAttributes())); $mailbox->setSpecialUse(json_encode($folder->getSpecialUse())); $mailbox->setMyAcls($folder->getMyAcls()); $mailbox->setShared($this->isMailboxShared($namespaces, $mailbox)); - $this->mailboxMapper->update($mailbox); + return $this->mailboxMapper->update($mailbox); } - private function createMailboxFromFolder(Account $account, Folder $folder, ?Horde_Imap_Client_Namespace_List $namespaces): void { + private function createMailboxFromFolder(Account $account, Folder $folder, ?Horde_Imap_Client_Namespace_List $namespaces): Mailbox { $mailbox = new Mailbox(); $mailbox->setName($folder->getMailbox()); $mailbox->setAccountId($account->getId()); $mailbox->setAttributes(json_encode($folder->getAttributes())); $mailbox->setDelimiter($folder->getDelimiter()); - $mailbox->setMessages($folder->getStatus()['messages'] ?? 0); - $mailbox->setUnseen($folder->getStatus()['unseen'] ?? 0); + $status = $folder->getStatus(); + $mailbox->setMessages($status['messages'] ?? 0); + $mailbox->setUnseen($status['unseen'] ?? 0); $mailbox->setSelectable(!in_array('\noselect', $folder->getAttributes())); $mailbox->setSpecialUse(json_encode($folder->getSpecialUse())); $mailbox->setMyAcls($folder->getMyAcls()); $mailbox->setShared($this->isMailboxShared($namespaces, $mailbox)); - $this->mailboxMapper->insert($mailbox); + return $this->mailboxMapper->insert($mailbox); } private function isMailboxShared(?Horde_Imap_Client_Namespace_List $namespaces, Mailbox $mailbox): bool { @@ -250,4 +266,41 @@ private function isMailboxShared(?Horde_Imap_Client_Namespace_List $namespaces, } return false; } + + private function syncMailboxStatus(mixed $mailboxes, ?string $personalNamespace, \Horde_Imap_Client_Socket $client): void { + /** @var array{0: Mailbox[], 1: Mailbox[]} */ + [$sync, $doNotSync] = array_reduce($mailboxes, function (array $carry, Mailbox $mailbox) use ($personalNamespace): array { + /** @var array{0: Mailbox[], 1: Mailbox[]} $carry */ + [$sync, $doNotSync] = $carry; + $inboxName = $personalNamespace === null ? "INBOX" : ($personalNamespace . $mailbox->getDelimiter() . "INBOX"); + if ($inboxName === $mailbox->getName() || $mailbox->getSyncInBackground()) { + return [ + array_merge($sync, [$mailbox]), + $doNotSync, + ]; + } + return [ + $sync, + array_merge($doNotSync, [$mailbox]), + ]; + }, [[], []]); + + // Synchronize the mailboxes selected for sync and keep the rest updated occasionally + shuffle($doNotSync); + /** @var Mailbox[] $syncStatus */ + $syncStatus = [...$sync, ...array_slice($doNotSync, 0, 5)]; + $statuses = $this->folderMapper->getFoldersStatusAsObject($client, array_map(function (Mailbox $mailbox) { + return $mailbox->getName(); + }, $syncStatus)); + foreach ($syncStatus as $mailbox) { + $status = $statuses[$mailbox->getName()]; + $mailbox->setMessages($status->getTotal()); + $mailbox->setUnseen($status->getUnread()); + } + $this->atomic(function () use ($syncStatus) { + foreach ($syncStatus as $mailbox) { + $this->mailboxMapper->update($mailbox); + } + }, $this->dbConnection); + } } diff --git a/tests/Unit/IMAP/FolderMapperTest.php b/tests/Unit/IMAP/FolderMapperTest.php index 070f84c34e..1137c59382 100644 --- a/tests/Unit/IMAP/FolderMapperTest.php +++ b/tests/Unit/IMAP/FolderMapperTest.php @@ -25,7 +25,6 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Horde_Imap_Client; -use Horde_Imap_Client_Data_Acl; use Horde_Imap_Client_Data_Capability; use Horde_Imap_Client_Mailbox; use Horde_Imap_Client_Socket; @@ -54,7 +53,6 @@ public function testGetFoldersEmtpyAccount(): void { 'delimiter' => true, 'attributes' => true, 'special_use' => true, - 'status' => Horde_Imap_Client::STATUS_ALL, ])) ->willReturn([]); @@ -74,16 +72,12 @@ public function testGetFoldersNonExistent(): void { 'delimiter' => true, 'attributes' => true, 'special_use' => true, - 'status' => Horde_Imap_Client::STATUS_ALL, ])) ->willReturn([ [ 'mailbox' => new Horde_Imap_Client_Mailbox('INBOX'), 'attributes' => [], 'delimiter' => '.', - 'status' => [ - 'unseen' => 0, - ], ], [ 'mailbox' => new Horde_Imap_Client_Mailbox('shared'), @@ -91,13 +85,10 @@ public function testGetFoldersNonExistent(): void { '\\nonexistent', ], 'delimiter' => '.', - 'status' => [ - 'unseen' => null, - ], ], ]); $expected = [ - new Folder(27, new Horde_Imap_Client_Mailbox('INBOX'), [], '.', ['unseen' => 0]), + new Folder(27, new Horde_Imap_Client_Mailbox('INBOX'), [], '.', null), ]; $folders = $this->mapper->getFolders($account, $client); @@ -116,16 +107,12 @@ public function testGetFolders(): void { 'delimiter' => true, 'attributes' => true, 'special_use' => true, - 'status' => Horde_Imap_Client::STATUS_ALL, ])) ->willReturn([ [ 'mailbox' => new Horde_Imap_Client_Mailbox('INBOX'), 'attributes' => [], 'delimiter' => '.', - 'status' => [ - 'unseen' => 0, - ], ], [ 'mailbox' => new Horde_Imap_Client_Mailbox('Sent'), @@ -133,14 +120,11 @@ public function testGetFolders(): void { '\sent', ], 'delimiter' => '.', - 'status' => [ - 'unseen' => 1, - ], ], ]); $expected = [ - new Folder(27, new Horde_Imap_Client_Mailbox('INBOX'), [], '.', ['unseen' => 0]), - new Folder(27, new Horde_Imap_Client_Mailbox('Sent'), ['\sent'], '.', ['unseen' => 1]), + new Folder(27, new Horde_Imap_Client_Mailbox('INBOX'), [], '.', null), + new Folder(27, new Horde_Imap_Client_Mailbox('Sent'), ['\sent'], '.', null), ]; $folders = $this->mapper->getFolders($account, $client); @@ -233,57 +217,19 @@ public function testGetFoldersStatusAsObject(): void { $client = $this->createMock(Horde_Imap_Client_Socket::class); $client->expects($this->once()) ->method('status') - ->with('INBOX') + ->with(['INBOX']) ->willReturn([ - 'messages' => 123, - 'unseen' => 2, - ]); - $capability = $this->createMock(Horde_Imap_Client_Data_Capability::class); - $client - ->method('__get') - ->with('capability') - ->willReturn($capability); - $capability - ->method('query') - ->with('ACL') - ->willReturn(false); - - $stats = $this->mapper->getFoldersStatusAsObject($client, 'INBOX'); - - $expected = new MailboxStats(123, 2, null); - $this->assertEquals($expected, $stats); - } - - public function testGetFoldersStatusAndMyAcls(): void { - $client = $this->createMock(Horde_Imap_Client_Socket::class); - $client->expects($this->once()) - ->method('status') - ->with('INBOX') - ->willReturn([ - 'messages' => 123, - 'unseen' => 2, + 'INBOX' => [ + 'messages' => 123, + 'unseen' => 2, + ], ]); - $capability = $this->createMock(Horde_Imap_Client_Data_Capability::class); - $client - ->method('__get') - ->with('capability') - ->willReturn($capability); - $capability - ->method('query') - ->with('ACL') - ->willReturn(true); - $acl = $this->createMock(Horde_Imap_Client_Data_Acl::class); - $client->expects(self::once()) - ->method('getMyACLRights') - ->willReturn($acl); - $acl - ->method('__toString') - ->willReturn('kthxbye'); - $stats = $this->mapper->getFoldersStatusAsObject($client, 'INBOX'); + $stats = $this->mapper->getFoldersStatusAsObject($client, ['INBOX']); - $expected = new MailboxStats(123, 2, 'kthxbye'); - $this->assertEquals($expected, $stats); + self::assertArrayHasKey('INBOX', $stats); + $expected = new MailboxStats(123, 2); + self::assertEquals($expected, $stats['INBOX']); } public function testDetectSpecialUseFromAttributes() { diff --git a/tests/Unit/IMAP/MailboxSyncTest.php b/tests/Unit/IMAP/MailboxSyncTest.php index c59c29fd21..5c145e93e0 100644 --- a/tests/Unit/IMAP/MailboxSyncTest.php +++ b/tests/Unit/IMAP/MailboxSyncTest.php @@ -107,7 +107,7 @@ public function testSyncSkipped() { $this->sync->sync($account, new NullLogger()); } - public function testSync() { + public function testSync(): void { $account = $this->createMock(Account::class); $mailAccount = new MailAccount(); $mailAccount->setLastMailboxSync(0); @@ -130,14 +130,29 @@ public function testSync() { 'messages' => 42, ]; $folders[0]->method('getStatus')->willReturn($status); + $folders[0]->method('getMailbox')->willReturn('mb1'); $folders[1]->method('getStatus')->willReturn($status); + $folders[1]->method('getMailbox')->willReturn('mb2'); $this->folderMapper->expects($this->once()) ->method('getFolders') ->with($account, $client) ->willReturn($folders); + $this->folderMapper->expects($this->once()) + ->method('getFoldersStatusAsObject') + ->with($client, self::equalToCanonicalizing(['mb1', 'mb2',])) + ->willReturn([ + 'mb1' => new MailboxStats(1, 2), + 'mb2' => new MailboxStats(1, 2), + ]); $this->folderMapper->expects($this->once()) ->method('detectFolderSpecialUse') ->with($folders); + $this->mailboxMapper->expects(self::exactly(2)) + ->method('insert') + ->willReturnArgument(0); + $this->mailboxMapper->expects(self::exactly(2)) + ->method('update') + ->willReturnArgument(0); $this->dispatcher ->expects($this->once()) ->method('dispatchTyped') @@ -195,6 +210,16 @@ public function testSyncShared(): void { ->method('findAll') ->with($account) ->willReturn([$inbox, $sharedMailbox]); + $this->folderMapper->expects($this->once()) + ->method('getFoldersStatusAsObject') + ->with($client, [$inbox->getName(), $sharedMailbox->getName()]) + ->willReturn([ + $inbox->getName() => new MailboxStats(1, 2), + $sharedMailbox->getName() => new MailboxStats(0, 0), + ]); + $this->mailboxMapper->expects(self::exactly(4)) + ->method('update') + ->willReturnArgument(0); $this->sync->sync($account, new NullLogger()); } @@ -211,8 +236,8 @@ public function testSyncStats(): void { $mailbox->setName('mailbox'); $this->folderMapper->expects($this->once()) ->method('getFoldersStatusAsObject') - ->with($client, $mailbox->getName()) - ->willReturn($stats); + ->with($client, [$mailbox->getName()]) + ->willReturn(['mailbox' => $stats]); $this->mailboxMapper->expects($this->once()) ->method('update') ->with($mailbox);