From 4fd06097bd688b046abfbfb18c7ca1053a23f2ab Mon Sep 17 00:00:00 2001 From: SebastianKrupinski Date: Thu, 8 Oct 2026 10:22:05 -0400 Subject: [PATCH] perf(imap): reuse one IMAP connection per account within a process Signed-off-by: SebastianKrupinski --- lib/BackgroundJob/MigrateImportantJob.php | 30 ++- lib/Command/TestAccount.php | 2 +- lib/IMAP/HordeImapClient.php | 4 + lib/IMAP/IMAPClientFactory.php | 4 +- lib/IMAP/ImapMailboxConnector.php | 18 +- lib/IMAP/ImapMessageConnector.php | 158 ++++++---------- lib/IMAP/ImapTransmissionConnector.php | 18 +- lib/IMAP/MailboxSync.php | 89 ++++----- lib/IMAP/PreviewEnhancer.php | 2 - lib/Protocol/ConnectionPool.php | 74 +++++++- lib/Protocol/ProtocolFactory.php | 24 ++- lib/Service/AntiSpamService.php | 18 +- lib/Service/SetupService.php | 2 +- lib/Service/Sync/ImapToDbSynchronizer.php | 71 ++++--- lib/SetupChecks/MailConnectionPerformance.php | 4 +- tests/Unit/IMAP/ImapMessageConnectorTest.php | 30 +-- .../IMAP/ImapTransmissionConnectorTest.php | 82 +++++++++ tests/Unit/IMAP/MailboxSyncTest.php | 2 +- tests/Unit/Protocol/ConnectionPoolTest.php | 174 +++++++++++++++++- tests/Unit/Protocol/ProtocolFactoryTest.php | 83 ++++++++- tests/Unit/Service/AntiSpamServiceTest.php | 4 +- tests/Unit/Service/SetupServiceTest.php | 6 +- .../MailConnectionPerformanceTest.php | 10 +- 23 files changed, 621 insertions(+), 288 deletions(-) diff --git a/lib/BackgroundJob/MigrateImportantJob.php b/lib/BackgroundJob/MigrateImportantJob.php index 59a86aafec..7a8f3bab7f 100644 --- a/lib/BackgroundJob/MigrateImportantJob.php +++ b/lib/BackgroundJob/MigrateImportantJob.php @@ -60,25 +60,21 @@ public function run($argument) { $account = new Account($mailAccount); $client = $this->protocolFactory->imapClient($account); - try { - if ($this->mailManager->isPermflagsEnabled($account, $mailbox) === false) { - $this->logger->debug("Permflags not enabled for <{$accountId}>"); - return; - } + if ($this->mailManager->isPermflagsEnabled($account, $mailbox) === false) { + $this->logger->debug("Permflags not enabled for <{$accountId}>"); + return; + } - try { - $this->migration->migrateImportantOnImap($client, $account, $mailbox); - } catch (ServiceException $e) { - $this->logger->debug("Could not flag messages on IMAP for mailbox <{$mailboxId}>."); - } + try { + $this->migration->migrateImportantOnImap($client, $account, $mailbox); + } catch (ServiceException $e) { + $this->logger->debug("Could not flag messages on IMAP for mailbox <{$mailboxId}>."); + } - try { - $this->migration->migrateImportantFromDb($client, $account, $mailbox); - } catch (ServiceException $e) { - $this->logger->debug("Could not flag messages from DB on IMAP for mailbox <{$mailboxId}>."); - } - } finally { - $client->logout(); + try { + $this->migration->migrateImportantFromDb($client, $account, $mailbox); + } catch (ServiceException $e) { + $this->logger->debug("Could not flag messages from DB on IMAP for mailbox <{$mailboxId}>."); } } } diff --git a/lib/Command/TestAccount.php b/lib/Command/TestAccount.php index 8f11e7f309..82e9c79271 100644 --- a/lib/Command/TestAccount.php +++ b/lib/Command/TestAccount.php @@ -151,7 +151,7 @@ private function testImap(Account $account, SymfonyStyle $io, int $mailboxLimit, $io->text('Opening IMAP connection...'); try { - $imapClient = $this->protocolFactory->imapClient($account); + $imapClient = $this->protocolFactory->newImapClient($account); } catch (\Exception $e) { $io->error('Could not create IMAP client: ' . $e->getMessage()); return self::FAILURE; diff --git a/lib/IMAP/HordeImapClient.php b/lib/IMAP/HordeImapClient.php index b9701becce..d2468e517f 100644 --- a/lib/IMAP/HordeImapClient.php +++ b/lib/IMAP/HordeImapClient.php @@ -46,6 +46,10 @@ public function enableRateLimiter( $this->hash = $hash; } + public function isConnectionLost(): bool { + return $this->_connection !== null && !$this->_connection->connected; + } + #[\Override] public function login() { $initiallyAutheticated = $this->_isAuthenticated; diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php index 40b86bfcca..4baf987fe1 100644 --- a/lib/IMAP/IMAPClientFactory.php +++ b/lib/IMAP/IMAPClientFactory.php @@ -11,7 +11,6 @@ use Exception; use Horde_Imap_Client_Password_Xoauth2; -use Horde_Imap_Client_Socket; use OCA\Mail\Account; use OCA\Mail\Cache\HordeCacheFactory; use OCA\Mail\Events\BeforeImapClientCreated; @@ -69,10 +68,9 @@ public function __construct( * @param Account $account * @param bool $useCache * - * @return Horde_Imap_Client_Socket * @throws ServiceException */ - public function getClient(Account $account, bool $useCache = true): Horde_Imap_Client_Socket { + public function getClient(Account $account, bool $useCache = true): HordeImapClient { $this->eventDispatcher->dispatchTyped( new BeforeImapClientCreated($account) ); diff --git a/lib/IMAP/ImapMailboxConnector.php b/lib/IMAP/ImapMailboxConnector.php index 7be26dffc6..558fffec51 100644 --- a/lib/IMAP/ImapMailboxConnector.php +++ b/lib/IMAP/ImapMailboxConnector.php @@ -37,11 +37,7 @@ public function syncAll(Account $account, bool $force = false): void { #[\Override] public function syncOne(Account $account, Mailbox $mailbox): void { $client = $this->protocolFactory->imapClient($account); - try { - $this->mailboxSync->syncStats($client, $mailbox); - } finally { - $client->logout(); - } + $this->mailboxSync->syncStats($client, $mailbox); } #[\Override] @@ -58,8 +54,6 @@ public function create(Account $account, string $name, array $specialUse = []): $e->getCode(), $e, ); - } finally { - $client->logout(); } return $this->mailboxMapper->find($account, $name); @@ -77,8 +71,6 @@ public function rename(Account $account, Mailbox $mailbox, string $newName): Mai $e->getCode(), $e, ); - } finally { - $client->logout(); } try { @@ -91,11 +83,7 @@ public function rename(Account $account, Mailbox $mailbox, string $newName): Mai #[\Override] public function delete(Account $account, Mailbox $mailbox): void { $client = $this->protocolFactory->imapClient($account); - try { - $this->folderMapper->delete($client, $mailbox->getName()); - } finally { - $client->logout(); - } + $this->folderMapper->delete($client, $mailbox->getName()); $this->mailboxMapper->delete($mailbox); } @@ -112,8 +100,6 @@ public function subscribe(Account $account, Mailbox $mailbox, bool $subscribed): $e->getCode(), $e, ); - } finally { - $client->logout(); } return $this->mailboxMapper->find($account, $mailbox->getName()); diff --git a/lib/IMAP/ImapMessageConnector.php b/lib/IMAP/ImapMessageConnector.php index b9891714df..25f84069f7 100644 --- a/lib/IMAP/ImapMessageConnector.php +++ b/lib/IMAP/ImapMessageConnector.php @@ -56,19 +56,15 @@ public function syncAll(Account $account, bool $force = false): void { #[\Override] public function syncMailbox(Account $account, Mailbox $mailbox, LoggerInterface $logger, int $criteria, ?array $knownUids = null, bool $force = false): SyncResult { $client = $this->protocolFactory->imapClient($account); - try { - $rebuildThreads = $this->synchronizer->sync( - $account, - $client, - $mailbox, - $logger, - $criteria, - $knownUids, - $force, - ); - } finally { - $client->logout(); - } + $rebuildThreads = $this->synchronizer->sync( + $account, + $client, + $mailbox, + $logger, + $criteria, + $knownUids, + $force, + ); return new SyncResult( state: $mailbox->getSyncChangedToken(), @@ -97,8 +93,6 @@ public function fetchMessages(Account $account, Mailbox $mailbox, bool $loadBody ); } catch (DoesNotExistException|Horde_Mime_Exception|Horde_Imap_Client_Exception $e) { throw new ServiceException('Could not load messages: ' . $e->getMessage(), $e->getCode(), $e); - } finally { - $client->logout(); } } @@ -112,8 +106,6 @@ public function findMessages(Account $account, Mailbox $mailbox, SearchQuery $se ); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException('Could not get message IDs: ' . $e->getMessage(), 0, $e); - } finally { - $client->logout(); } return $fetchResult['match']->ids; @@ -122,17 +114,13 @@ public function findMessages(Account $account, Mailbox $mailbox, SearchQuery $se #[\Override] public function fetchMessageRaw(Account $account, Mailbox $mailbox, Message $message, bool $decrypt = false): ?string { $client = $this->protocolFactory->imapClient($account); - try { - return $this->imapMessageMapper->getFullText( - $client, - $mailbox->getName(), - $message->getUid(), - $account->getUserId(), - $decrypt, - ); - } finally { - $client->logout(); - } + return $this->imapMessageMapper->getFullText( + $client, + $mailbox->getName(), + $message->getUid(), + $account->getUserId(), + $decrypt, + ); } /** @@ -152,8 +140,6 @@ public function fetchAttachments(Account $account, Mailbox $mailbox, Message $me ); } catch (Horde_Imap_Client_Exception_NoSupportExtension|Horde_Imap_Client_Exception|Horde_Mime_Exception $e) { throw new ServiceException('Could not load attachments from IMAP: ' . $e->getMessage(), $e->getCode(), $e); - } finally { - $client->logout(); } } @@ -174,8 +160,6 @@ public function fetchAttachment(Account $account, Mailbox $mailbox, Message $mes ); } catch (Horde_Imap_Client_Exception|Horde_Mime_Exception $e) { throw new ServiceException('Could not load attachment from IMAP: ' . $e->getMessage(), $e->getCode(), $e); - } finally { - $client->logout(); } } @@ -187,38 +171,34 @@ public function moveMessages(Account $account, Mailbox $targetMailbox, Mailbox $ $client = $this->protocolFactory->imapClient($account); $mutatedMessages = []; - try { - foreach ($messages as $message) { - try { - $newUid = $this->imapMessageMapper->move($client, $sourceMailbox->getName(), $message->getUid(), $targetMailbox->getName()); - if ($newUid === null) { - // The IMAP server does not support UIDPLUS and the message has no Message-ID - // header, so the new UID is unknown. It will be reconciled on the next sync. - $this->logger->debug('Moved message but could not determine its new UID', [ - 'userId' => $account->getUserId(), - 'accountId' => $account->getId(), - 'sourceMailboxId' => $sourceMailbox->getId(), - 'targetMailboxId' => $targetMailbox->getId(), - 'messageUid' => $message->getUid(), - ]); - continue; - } - $message->setUid($newUid); - $message->setMailboxId($targetMailbox->getId()); - $mutatedMessages[] = $message; - } catch (Horde_Imap_Client_Exception $e) { - $this->logger->error('Could not move message on remote IMAP server', [ - 'exception' => $e, + foreach ($messages as $message) { + try { + $newUid = $this->imapMessageMapper->move($client, $sourceMailbox->getName(), $message->getUid(), $targetMailbox->getName()); + if ($newUid === null) { + // The IMAP server does not support UIDPLUS and the message has no Message-ID + // header, so the new UID is unknown. It will be reconciled on the next sync. + $this->logger->debug('Moved message but could not determine its new UID', [ 'userId' => $account->getUserId(), 'accountId' => $account->getId(), 'sourceMailboxId' => $sourceMailbox->getId(), 'targetMailboxId' => $targetMailbox->getId(), 'messageUid' => $message->getUid(), ]); + continue; } + $message->setUid($newUid); + $message->setMailboxId($targetMailbox->getId()); + $mutatedMessages[] = $message; + } catch (Horde_Imap_Client_Exception $e) { + $this->logger->error('Could not move message on remote IMAP server', [ + 'exception' => $e, + 'userId' => $account->getUserId(), + 'accountId' => $account->getId(), + 'sourceMailboxId' => $sourceMailbox->getId(), + 'targetMailboxId' => $targetMailbox->getId(), + 'messageUid' => $message->getUid(), + ]); } - } finally { - $client->logout(); } return $mutatedMessages; @@ -232,23 +212,19 @@ public function deleteMessages(Account $account, Mailbox $mailbox, Message ...$m $client = $this->protocolFactory->imapClient($account); $mutatedMessages = []; - try { - foreach ($messages as $message) { - try { - $this->imapMessageMapper->expunge($client, $mailbox->getName(), $message->getUid()); - $mutatedMessages[] = $message; - } catch (Horde_Imap_Client_Exception $e) { - $this->logger->error('Could not delete message on remote IMAP server', [ - 'exception' => $e, - 'userId' => $account->getUserId(), - 'accountId' => $account->getId(), - 'mailboxId' => $mailbox->getId(), - 'messageUid' => $message->getUid(), - ]); - } + foreach ($messages as $message) { + try { + $this->imapMessageMapper->expunge($client, $mailbox->getName(), $message->getUid()); + $mutatedMessages[] = $message; + } catch (Horde_Imap_Client_Exception $e) { + $this->logger->error('Could not delete message on remote IMAP server', [ + 'exception' => $e, + 'userId' => $account->getUserId(), + 'accountId' => $account->getId(), + 'mailboxId' => $mailbox->getId(), + 'messageUid' => $message->getUid(), + ]); } - } finally { - $client->logout(); } return $mutatedMessages; @@ -281,8 +257,6 @@ public function flagMessages(Account $account, Mailbox $mailbox, string $flag, b } } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException('Could not set message flag on remote IMAP server: ' . $e->getMessage(), $e->getCode(), $e); - } finally { - $client->logout(); } return $messages; @@ -295,24 +269,20 @@ public function tagMessages(Account $account, Mailbox $mailbox, Tag $tag, bool $ } $client = $this->protocolFactory->imapClient($account); - try { - if ($this->isPermflagsEnabledWithClient($client, $mailbox->getName()) === false) { - $this->logger->error('Cannot set message keyword, server does not support permanent flags', ['tag' => $tag->getDisplayName()]); - return []; - } + if ($this->isPermflagsEnabledWithClient($client, $mailbox->getName()) === false) { + $this->logger->error('Cannot set message keyword, server does not support permanent flags', ['tag' => $tag->getDisplayName()]); + return []; + } - $uids = array_map(static fn (Message $message) => $message->getUid(), $messages); - try { - if ($value) { - $this->imapMessageMapper->addFlag($client, $mailbox, $uids, $tag->getImapLabel()); - } else { - $this->imapMessageMapper->removeFlag($client, $mailbox, $uids, $tag->getImapLabel()); - } - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException('Could not set message keyword on remote IMAP server: ' . $e->getMessage(), $e->getCode(), $e); + $uids = array_map(static fn (Message $message) => $message->getUid(), $messages); + try { + if ($value) { + $this->imapMessageMapper->addFlag($client, $mailbox, $uids, $tag->getImapLabel()); + } else { + $this->imapMessageMapper->removeFlag($client, $mailbox, $uids, $tag->getImapLabel()); } - } finally { - $client->logout(); + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException('Could not set message keyword on remote IMAP server: ' . $e->getMessage(), $e->getCode(), $e); } foreach ($messages as $message) { @@ -334,8 +304,6 @@ public function getQuota(Account $account): ?Quota { return null; } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException('Could not get quota from IMAP: ' . $e->getMessage(), $e->getCode(), $e); - } finally { - $client->logout(); } $storageQuotas = array_map(static fn (array $root) => $root['storage'] ?? [ @@ -368,11 +336,7 @@ public function repairSync(Account $account, Mailbox $mailbox): void { #[\Override] public function isPermflagsEnabled(Account $account, Mailbox $mailbox): bool { $client = $this->protocolFactory->imapClient($account); - try { - return $this->isPermflagsEnabledWithClient($client, $mailbox->getName()); - } finally { - $client->logout(); - } + return $this->isPermflagsEnabledWithClient($client, $mailbox->getName()); } private function isPermflagsEnabledWithClient($client, string $mailbox): bool { diff --git a/lib/IMAP/ImapTransmissionConnector.php b/lib/IMAP/ImapTransmissionConnector.php index 505fbc6191..850a49f9d4 100644 --- a/lib/IMAP/ImapTransmissionConnector.php +++ b/lib/IMAP/ImapTransmissionConnector.php @@ -88,8 +88,6 @@ public function sendMessage(Account $account, LocalMessage $message, Mailbox $se } catch (\Throwable $e) { $this->logger->error('Retry copy-to-sent failed: ' . $e->getMessage(), ['exception' => $e]); $message->setStatus(LocalMessage::STATUS_IMAP_SENT_MAILBOX_FAIL); - } finally { - $client->logout(); } } else { $message->setStatus(LocalMessage::STATUS_ERROR); @@ -244,8 +242,6 @@ public function sendMessage(Account $account, LocalMessage $message, Mailbox $se } catch (\Throwable $e) { $this->logger->error('Copy to sent mailbox failed: ' . $e->getMessage(), ['exception' => $e]); $message->setStatus(LocalMessage::STATUS_IMAP_SENT_MAILBOX_FAIL); - } finally { - $client->logout(); } } } @@ -311,8 +307,6 @@ public function saveMessage(Account $account, Mailbox $mailbox, LocalMessage $me $perfLogger->step('save message on IMAP'); } catch (Horde_Exception $e) { throw new ServiceException('Could not save message to IMAP mailbox', 0, $e); - } finally { - $client->logout(); } $perfLogger->end(); @@ -330,14 +324,10 @@ public function sendMdn(Account $account, Mailbox $mailbox, Message $message): v ]); $imapClient = $this->protocolFactory->imapClient($account); - try { - /** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */ - $fetchResults = iterator_to_array($imapClient->fetch($mailbox->getName(), $query, [ - 'ids' => new Horde_Imap_Client_Ids([$message->getUid()]), - ]), false); - } finally { - $imapClient->logout(); - } + /** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */ + $fetchResults = iterator_to_array($imapClient->fetch($mailbox->getName(), $query, [ + 'ids' => new Horde_Imap_Client_Ids([$message->getUid()]), + ]), false); if (count($fetchResults) < 1) { throw new ServiceException("Message \"{$message->getId()}\" not found."); diff --git a/lib/IMAP/MailboxSync.php b/lib/IMAP/MailboxSync.php index 90ed10398d..f4074ff7e3 100644 --- a/lib/IMAP/MailboxSync.php +++ b/lib/IMAP/MailboxSync.php @@ -78,62 +78,51 @@ public function sync(Account $account, return; } - if ($client === null) { - $client = $this->protocolFactory->imapClient($account); - $ownClient = true; - } else { - $ownClient = false; - } + $client ??= $this->protocolFactory->imapClient($account); try { - try { - $namespaces = $client->getNamespaces([], [ - 'ob_return' => true, - ]); - $personalNamespace = $this->getPersonalNamespace($namespaces); - $account->getMailAccount()->setPersonalNamespace( - $personalNamespace - ); - } catch (Horde_Imap_Client_Exception $e) { - $id = $account->getId(); - $logger->debug("Getting namespaces for account $id failed: " . $e->getMessage(), [ - 'exception' => $e, - ]); - $namespaces = null; - $personalNamespace = null; - } + $namespaces = $client->getNamespaces([], [ + 'ob_return' => true, + ]); + $personalNamespace = $this->getPersonalNamespace($namespaces); + $account->getMailAccount()->setPersonalNamespace( + $personalNamespace + ); + } catch (Horde_Imap_Client_Exception $e) { + $id = $account->getId(); + $logger->debug("Getting namespaces for account $id failed: " . $e->getMessage(), [ + 'exception' => $e, + ]); + $namespaces = null; + $personalNamespace = null; + } - try { - $folders = $this->folderMapper->getFolders($account, $client); - $this->folderMapper->fetchFolderAcls($folders, $client); - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException( - sprintf('IMAP error synchronizing account %d: %s', $account->getId(), $e->getMessage()), - $e->getCode(), - $e - ); - } - $this->folderMapper->detectFolderSpecialUse($folders); + try { + $folders = $this->folderMapper->getFolders($account, $client); + $this->folderMapper->fetchFolderAcls($folders, $client); + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException( + sprintf('IMAP error synchronizing account %d: %s', $account->getId(), $e->getMessage()), + $e->getCode(), + $e + ); + } + $this->folderMapper->detectFolderSpecialUse($folders); - $mailboxes = $this->atomic(function () use ($account, $folders, $namespaces) { - $old = $this->mailboxMapper->findAll($account); - $indexedOld = array_combine( - array_map(static fn (Mailbox $mb) => $mb->getName(), $old), - $old - ); + $mailboxes = $this->atomic(function () use ($account, $folders, $namespaces) { + $old = $this->mailboxMapper->findAll($account); + $indexedOld = array_combine( + array_map(static fn (Mailbox $mb) => $mb->getName(), $old), + $old + ); - return $this->persist($account, $folders, $indexedOld, $namespaces); - }, $this->dbConnection); + return $this->persist($account, $folders, $indexedOld, $namespaces); + }, $this->dbConnection); - $this->syncMailboxStatus($mailboxes, $personalNamespace, $client); + $this->syncMailboxStatus($mailboxes, $personalNamespace, $client); - $this->dispatcher->dispatchTyped( - new MailboxesSynchronizedEvent($account) - ); - } finally { - if ($ownClient) { - $client->logout(); - } - } + $this->dispatcher->dispatchTyped( + new MailboxesSynchronizedEvent($account) + ); } /** diff --git a/lib/IMAP/PreviewEnhancer.php b/lib/IMAP/PreviewEnhancer.php index 929a85a946..b87a4de1a1 100644 --- a/lib/IMAP/PreviewEnhancer.php +++ b/lib/IMAP/PreviewEnhancer.php @@ -105,8 +105,6 @@ public function process(Account $account, Mailbox $mailbox, array $messages, boo ]); return $messages; - } finally { - $client->logout(); } return $this->mapper->updatePreviewDataBulk(...array_map(static function (Message $message) use ($data) { diff --git a/lib/Protocol/ConnectionPool.php b/lib/Protocol/ConnectionPool.php index e4705a3b20..4c8afc0f5f 100644 --- a/lib/Protocol/ConnectionPool.php +++ b/lib/Protocol/ConnectionPool.php @@ -9,26 +9,64 @@ namespace OCA\Mail\Protocol; +use Horde_Imap_Client_Exception; use JmapClient\Client as JmapClient; use OCA\Mail\Account; use OCA\Mail\Exception\ServiceException; +use OCA\Mail\IMAP\HordeImapClient; +use OCA\Mail\IMAP\IMAPClientFactory; use OCA\Mail\JMAP\JmapClientFactory; +use Psr\Log\LoggerInterface; use function hash; use function json_encode; +use function register_shutdown_function; /** * Long-running processes that handle many accounts must release each * account once done, or connections accumulate for the whole process. */ class ConnectionPool { + /** @var array> */ + private array $imapClients = []; /** @var array */ private array $jmapClients = []; + private bool $shutdownRegistered = false; public function __construct( + private IMAPClientFactory $imapClientFactory, private JmapClientFactory $jmapClientFactory, + private LoggerInterface $logger, ) { } + /** + * @throws ServiceException + */ + public function imap(Account $account, bool $useCache = true): HordeImapClient { + $id = $account->getId(); + $variant = $useCache ? 'cache' : 'nocache'; + + $entry = $this->imapClients[$id][$variant] ?? null; + if ($entry !== null && $entry['fingerprint'] === $this->fingerprint($account)) { + if ($entry['client']->isConnectionLost()) { + $entry['client']->logout(); + } + return $entry['client']; + } + if ($entry !== null) { + $this->logout($entry['client']); + } + + $client = $this->imapClientFactory->getClient($account, $useCache); + // Creating the client may refresh the account's OAuth token + $this->imapClients[$id][$variant] = [ + 'fingerprint' => $this->fingerprint($account), + 'client' => $client, + ]; + $this->registerShutdown(); + return $client; + } + /** * @throws ServiceException */ @@ -50,7 +88,39 @@ public function jmap(Account $account): JmapClient { } public function release(Account $account): void { - unset($this->jmapClients[$account->getId()]); + $id = $account->getId(); + foreach ($this->imapClients[$id] ?? [] as $entry) { + $this->logout($entry['client']); + } + unset($this->imapClients[$id], $this->jmapClients[$id]); + } + + public function releaseAll(): void { + foreach ($this->imapClients as $entries) { + foreach ($entries as $entry) { + $this->logout($entry['client']); + } + } + $this->imapClients = []; + $this->jmapClients = []; + } + + private function logout(HordeImapClient $client): void { + try { + $client->logout(); + } catch (Horde_Imap_Client_Exception $e) { + $this->logger->debug('Could not log out of IMAP connection: ' . $e->getMessage(), [ + 'exception' => $e, + ]); + } + } + + private function registerShutdown(): void { + if ($this->shutdownRegistered) { + return; + } + register_shutdown_function($this->releaseAll(...)); + $this->shutdownRegistered = true; } private function fingerprint(Account $account): string { @@ -64,6 +134,8 @@ private function fingerprint(Account $account): string { $mailAccount->getInboundUser(), $mailAccount->getInboundPassword(), $mailAccount->getAuthMethod(), + $mailAccount->getOauthAccessToken(), + $mailAccount->getDebug(), ], JSON_THROW_ON_ERROR)); } } diff --git a/lib/Protocol/ProtocolFactory.php b/lib/Protocol/ProtocolFactory.php index d12ef0dac3..765b215f99 100644 --- a/lib/Protocol/ProtocolFactory.php +++ b/lib/Protocol/ProtocolFactory.php @@ -56,11 +56,26 @@ public function __construct( } /** + * Get the account's shared IMAP client + * + * Callers must not log out; the connection is closed when the account is + * released or the process ends. + * * @throws ServiceException */ public function imapClient(Account $account, bool $useCache = true): Horde_Imap_Client_Socket { $this->verifyProtocol($account, MailAccount::PROTOCOL_IMAP); - return $this->imapClientFactory->getClient($account, $useCache); + return $this->connectionPool->imap($account, $useCache); + } + + /** + * Get a new IMAP client that is not shared, the caller must log out + * + * @throws ServiceException + */ + public function newImapClient(Account $account): Horde_Imap_Client_Socket { + $this->verifyProtocol($account, MailAccount::PROTOCOL_IMAP); + return $this->imapClientFactory->getClient($account); } /** @@ -86,7 +101,12 @@ public function testConnection(Account $account): void { $protocol = $account->getMailAccount()->getProtocol(); if ($protocol === MailAccount::PROTOCOL_IMAP) { - $this->imapClient($account)->close(); + $client = $this->newImapClient($account); + try { + $client->login(); + } finally { + $client->logout(); + } return; } diff --git a/lib/Service/AntiSpamService.php b/lib/Service/AntiSpamService.php index c0ec727249..6bf287a770 100644 --- a/lib/Service/AntiSpamService.php +++ b/lib/Service/AntiSpamService.php @@ -121,16 +121,12 @@ public function sendReportEmail(Account $account, Mailbox $mailbox, int $uid, st $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); $client = $this->protocolFactory->imapClient($account); - try { - $fullText = $this->messageMapper->getFullText( - $client, - $mailbox->getName(), - $attachmentMessage->getUid(), - $userId - ); - } finally { - $client->logout(); - } + $fullText = $this->messageMapper->getFullText( + $client, + $mailbox->getName(), + $attachmentMessage->getUid(), + $userId + ); $message->addEmbeddedMessageAttachment( $attachmentMessage->getSubject() . '.eml', @@ -207,8 +203,6 @@ public function sendReportEmail(Account $account, Mailbox $mailbox, int $uid, st ); } catch (Horde_Imap_Client_Exception $e) { $this->logger->error("Could not move report email to sent mailbox, but the report email was sent. Reported email was id: #$messageId", ['exception' => $e]); - } finally { - $client->logout(); } } diff --git a/lib/Service/SetupService.php b/lib/Service/SetupService.php index e7ce92aec2..8b7de389b9 100644 --- a/lib/Service/SetupService.php +++ b/lib/Service/SetupService.php @@ -109,7 +109,7 @@ public function createNewAccount(string $accountName, protected function testConnectivity(Account $account): void { $mailAccount = $account->getMailAccount(); - $imapClient = $this->protocolFactory->imapClient($account); + $imapClient = $this->protocolFactory->newImapClient($account); try { $imapClient->login(); } catch (Horde_Imap_Client_Exception $e) { diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index 3efa591552..fdfad52a95 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -110,8 +110,6 @@ public function syncAccount(Account $account, } } - $client->logout(); - $this->dispatcher->dispatchTyped( new SynchronizationEvent( $account, @@ -302,53 +300,47 @@ private function runInitialSync( // Use a no-cache client for findAll. Horde accumulates cache state on // every iteration of the findAll loop, causing a memory leak on large - // mailboxes (commit e50c214ff). Do NOT logout $client — the caller owns - // it and we need it below for getSyncToken. + // mailboxes (commit e50c214ff). $noCacheClient = $this->protocolFactory->imapClient($account, false); + $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); try { - $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); - try { - $imapMessages = $this->imapMapper->findAll( - $noCacheClient, - $mailbox->getName(), - self::MAX_NEW_MESSAGES, - $highestKnownUid ?? 0, - $logger, - $perf, - $account->getUserId(), - ); - $perf->step(sprintf('fetch %d messages from IMAP', count($imapMessages))); - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); - } + $imapMessages = $this->imapMapper->findAll( + $noCacheClient, + $mailbox->getName(), + self::MAX_NEW_MESSAGES, + $highestKnownUid ?? 0, + $logger, + $perf, + $account->getUserId(), + ); + $perf->step(sprintf('fetch %d messages from IMAP', count($imapMessages))); + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); + } - foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { - $messages = array_map(static fn (IMAPMessage $imapMessage) => $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()), $chunk); - $this->dbMapper->insertBulk($account, ...$messages); - $perf->step(sprintf('persist %d messages in database', count($chunk))); - // Free the memory - unset($messages); - } + foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { + $messages = array_map(static fn (IMAPMessage $imapMessage) => $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()), $chunk); + $this->dbMapper->insertBulk($account, ...$messages); + $perf->step(sprintf('persist %d messages in database', count($chunk))); + // Free the memory + unset($messages); + } - if (!$imapMessages['all']) { - // We might need more attempts to fill the cache - $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); - $total = $imapMessages['total']; - $cached = count($this->dbMapper->findAllUids($mailbox)); - $perf->step('find number of cached UIDs'); + if (!$imapMessages['all']) { + // We might need more attempts to fill the cache + $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); + $total = $imapMessages['total']; + $cached = count($this->dbMapper->findAllUids($mailbox)); + $perf->step('find number of cached UIDs'); - $perf->end(); - throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); - } - } finally { - $noCacheClient->logout(); + $perf->end(); + throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); } // Use the cache-enabled $client passed in for the sync token. The no-cache // client does not activate CONDSTORE/QRESYNC (commit 7980d9e40), so its // token lacks HIGHESTMODSEQ — causing the first partial sync to resolve ALL - // UIDs and run OOM on large mailboxes. Do NOT logout $client here; the - // caller (syncAccount) owns and closes it. + // UIDs and run OOM on large mailboxes. $syncToken = $client->getSyncToken($mailbox->getName()); $mailbox->setSyncNewToken($syncToken); $mailbox->setSyncChangedToken($syncToken); @@ -556,7 +548,6 @@ public function repairSync( throw new ServiceException($message, 0, $e); } finally { $this->mailboxMapper->unlockFromVanishedSync($mailbox); - $client->logout(); } $perf->end(); diff --git a/lib/SetupChecks/MailConnectionPerformance.php b/lib/SetupChecks/MailConnectionPerformance.php index aff68314a9..b04e718f5d 100644 --- a/lib/SetupChecks/MailConnectionPerformance.php +++ b/lib/SetupChecks/MailConnectionPerformance.php @@ -59,7 +59,7 @@ public function run(): SetupResult { foreach ($collection as $accountId) { $account = new Account($this->accountMapper->findById((int)$accountId)); try { - $client = $this->protocolFactory->imapClient($account); + $client = $this->protocolFactory->newImapClient($account); } catch (ServiceException $e) { $this->logger->warning('Error occurred while getting IMAP client for setup check: ' . $e->getMessage(), [ 'exception' => $e, @@ -81,7 +81,7 @@ public function run(): SetupResult { } catch (Throwable $e) { $this->logger->warning("Error occurred while performing system check on mail account: {$account->getId()}"); } finally { - $client->close(); + $client->logout(); } } } diff --git a/tests/Unit/IMAP/ImapMessageConnectorTest.php b/tests/Unit/IMAP/ImapMessageConnectorTest.php index ad7e56f7b9..42db699fa8 100644 --- a/tests/Unit/IMAP/ImapMessageConnectorTest.php +++ b/tests/Unit/IMAP/ImapMessageConnectorTest.php @@ -90,14 +90,14 @@ public function testFetchMessagesEnablesPhishingCheck(bool $loadBody): void { ->method('findByIds') ->with($this->client, 'INBOX', [1, 2], 'user', $loadBody, true) ->willReturn($fetchedMessages); - $this->client->expects(self::once())->method('logout'); + $this->client->expects(self::never())->method('logout'); $result = $this->connector->fetchMessages($this->account, $mailbox, $loadBody, $message, $otherMessage); self::assertSame($fetchedMessages, $result); } - public function testMoveMessagesLogsOutClientWhenMapperThrows(): void { + public function testMoveMessagesKeepsSharedClientWhenMapperThrows(): void { $sourceMailbox = new Mailbox(); $sourceMailbox->setName('INBOX'); $targetMailbox = new Mailbox(); @@ -107,7 +107,7 @@ public function testMoveMessagesLogsOutClientWhenMapperThrows(): void { $this->imapMessageMapper->method('move') ->willThrowException(new ServiceException('could not move')); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $this->expectException(ServiceException::class); @@ -115,7 +115,7 @@ public function testMoveMessagesLogsOutClientWhenMapperThrows(): void { $this->connector->moveMessages($this->account, $targetMailbox, $sourceMailbox, $message); } - public function testDeleteMessagesLogsOutClientWhenMapperThrows(): void { + public function testDeleteMessagesKeepsSharedClientWhenMapperThrows(): void { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); $message = new Message(); @@ -123,7 +123,7 @@ public function testDeleteMessagesLogsOutClientWhenMapperThrows(): void { $this->imapMessageMapper->method('expunge') ->willThrowException(new ServiceException('could not expunge')); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $this->expectException(ServiceException::class); @@ -131,7 +131,7 @@ public function testDeleteMessagesLogsOutClientWhenMapperThrows(): void { $this->connector->deleteMessages($this->account, $mailbox, $message); } - public function testFlagMessagesLogsOutClientWhenMapperThrows(): void { + public function testFlagMessagesKeepsSharedClientWhenMapperThrows(): void { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); $message = new Message(); @@ -139,7 +139,7 @@ public function testFlagMessagesLogsOutClientWhenMapperThrows(): void { $this->imapMessageMapper->method('addFlag') ->willThrowException(new Horde_Imap_Client_Exception('store failed')); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $this->expectException(ServiceException::class); @@ -147,7 +147,7 @@ public function testFlagMessagesLogsOutClientWhenMapperThrows(): void { $this->connector->flagMessages($this->account, $mailbox, 'seen', true, $message); } - public function testTagMessagesLogsOutClientWhenPermflagsCheckThrows(): void { + public function testTagMessagesKeepsSharedClientWhenPermflagsCheckThrows(): void { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); $tag = new Tag(); @@ -158,7 +158,7 @@ public function testTagMessagesLogsOutClientWhenPermflagsCheckThrows(): void { $this->client->method('status') ->willThrowException(new Horde_Imap_Client_Exception('status failed')); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $this->expectException(ServiceException::class); @@ -166,7 +166,7 @@ public function testTagMessagesLogsOutClientWhenPermflagsCheckThrows(): void { $this->connector->tagMessages($this->account, $mailbox, $tag, true, $message); } - public function testTagMessagesLogsOutClientWhenPermflagsNotSupported(): void { + public function testTagMessagesKeepsSharedClientWhenPermflagsNotSupported(): void { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); $tag = new Tag(); @@ -177,7 +177,7 @@ public function testTagMessagesLogsOutClientWhenPermflagsNotSupported(): void { $this->client->method('status') ->willReturn(['permflags' => []]); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $result = $this->connector->tagMessages($this->account, $mailbox, $tag, true, $message); @@ -185,13 +185,13 @@ public function testTagMessagesLogsOutClientWhenPermflagsNotSupported(): void { self::assertSame([], $result); } - public function testIsPermflagsEnabledLogsOutClientWhenStatusThrows(): void { + public function testIsPermflagsEnabledKeepsSharedClientWhenStatusThrows(): void { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); $this->client->method('status') ->willThrowException(new Horde_Imap_Client_Exception('status failed')); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $this->expectException(ServiceException::class); @@ -199,13 +199,13 @@ public function testIsPermflagsEnabledLogsOutClientWhenStatusThrows(): void { $this->connector->isPermflagsEnabled($this->account, $mailbox); } - public function testIsPermflagsEnabledLogsOutClientOnSuccess(): void { + public function testIsPermflagsEnabledKeepsSharedClientOnSuccess(): void { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); $this->client->method('status') ->willReturn(['permflags' => ['\\*']]); - $this->client->expects(self::once()) + $this->client->expects(self::never()) ->method('logout'); $result = $this->connector->isPermflagsEnabled($this->account, $mailbox); diff --git a/tests/Unit/IMAP/ImapTransmissionConnectorTest.php b/tests/Unit/IMAP/ImapTransmissionConnectorTest.php index 3e04196f88..88a4136cb5 100644 --- a/tests/Unit/IMAP/ImapTransmissionConnectorTest.php +++ b/tests/Unit/IMAP/ImapTransmissionConnectorTest.php @@ -9,6 +9,8 @@ namespace OCA\Mail\Tests\Unit\IMAP; use ChristophWurst\Nextcloud\Testing\TestCase; +use Horde_Imap_Client_Exception; +use Horde_Imap_Client_Fetch_Results; use Horde_Imap_Client_Socket; use OCA\Mail\Account; use OCA\Mail\Address; @@ -18,6 +20,7 @@ use OCA\Mail\Db\MailAccount; use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\MailboxMapper; +use OCA\Mail\Db\Message; use OCA\Mail\Db\Recipient; use OCA\Mail\Db\SmimeCertificate; use OCA\Mail\Exception\AttachmentNotFoundException; @@ -491,4 +494,83 @@ public function testSaveMessageIncludesAttachments(): void { $this->assertStringContainsString('test.txt', $capturedRaw); $this->assertStringContainsString('Attachment contents', $capturedRaw); } + + private function imapAccount(): Account { + $mailAccount = new MailAccount(); + $mailAccount->setUserId('bob'); + $mailAccount->setName('Bob'); + $mailAccount->setEmail('bob@example.com'); + return new Account($mailAccount); + } + + private function sharedImapClient(): Horde_Imap_Client_Socket&MockObject { + $client = $this->createMock(Horde_Imap_Client_Socket::class); + $client->expects(self::never()) + ->method('logout'); + $this->protocolFactory->method('imapClient') + ->willReturn($client); + return $client; + } + + public function testRetryCopyToSentKeepsSharedClient(): void { + $this->sharedImapClient(); + $message = new LocalMessage(); + $message->setStatus(LocalMessage::STATUS_IMAP_SENT_MAILBOX_FAIL); + $message->setRaw('raw message'); + $this->messageMapper->expects(self::once()) + ->method('save'); + + $this->connector->sendMessage($this->imapAccount(), $message, new Mailbox()); + + self::assertSame(LocalMessage::STATUS_PROCESSED, $message->getStatus()); + } + + public function testRetryCopyToSentFailureKeepsSharedClient(): void { + $this->sharedImapClient(); + $message = new LocalMessage(); + $message->setStatus(LocalMessage::STATUS_IMAP_SENT_MAILBOX_FAIL); + $message->setRaw('raw message'); + $this->messageMapper->method('save') + ->willThrowException(new Horde_Imap_Client_Exception('Connection lost')); + + $this->connector->sendMessage($this->imapAccount(), $message, new Mailbox()); + + self::assertSame(LocalMessage::STATUS_IMAP_SENT_MAILBOX_FAIL, $message->getStatus()); + } + + public function testSaveMessageFailureKeepsSharedClient(): void { + $this->sharedImapClient(); + $mailbox = new Mailbox(); + $mailbox->setName('Drafts'); + $message = new LocalMessage(); + $message->setSubject('Hello'); + $message->setBodyPlain('Body'); + $message->setHtml(false); + $this->transmissionService->method('getAddressList') + ->willReturn(new AddressList([])); + $this->transmissionService->method('getAttachments') + ->willReturn([]); + $this->performanceLogger->method('start') + ->willReturn($this->createMock(PerformanceLoggerTask::class)); + $this->messageMapper->method('save') + ->willThrowException(new Horde_Imap_Client_Exception('APPEND failed')); + $this->expectException(ServiceException::class); + + $this->connector->saveMessage($this->imapAccount(), $mailbox, $message); + } + + public function testSendMdnKeepsSharedClient(): void { + $client = $this->sharedImapClient(); + $client->method('fetch') + ->willReturn(new Horde_Imap_Client_Fetch_Results()); + $mailbox = new Mailbox(); + $mailbox->setName('INBOX'); + $message = new Message(); + $message->setId(1); + $message->setUid(42); + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Message "1" not found.'); + + $this->connector->sendMdn($this->imapAccount(), $mailbox, $message); + } } diff --git a/tests/Unit/IMAP/MailboxSyncTest.php b/tests/Unit/IMAP/MailboxSyncTest.php index a09156f8f3..0e2d8ff779 100644 --- a/tests/Unit/IMAP/MailboxSyncTest.php +++ b/tests/Unit/IMAP/MailboxSyncTest.php @@ -304,7 +304,7 @@ public function testSyncWithoutClient(): void { ->method('imapClient') ->with($account) ->willReturn($client); - $client->expects($this->once()) + $client->expects($this->never()) ->method('logout'); $this->sync->sync($account, new NullLogger(), false); diff --git a/tests/Unit/Protocol/ConnectionPoolTest.php b/tests/Unit/Protocol/ConnectionPoolTest.php index b8b365bb4b..c90b20f4c2 100644 --- a/tests/Unit/Protocol/ConnectionPoolTest.php +++ b/tests/Unit/Protocol/ConnectionPoolTest.php @@ -10,31 +10,41 @@ namespace OCA\Mail\Tests\Unit\Protocol; use ChristophWurst\Nextcloud\Testing\TestCase; +use Horde_Imap_Client_Exception; use JmapClient\Client as JmapClient; use OCA\Mail\Account; use OCA\Mail\Db\MailAccount; use OCA\Mail\Exception\ServiceException; +use OCA\Mail\IMAP\HordeImapClient; +use OCA\Mail\IMAP\IMAPClientFactory; use OCA\Mail\JMAP\JmapClientFactory; use OCA\Mail\Protocol\ConnectionPool; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; class ConnectionPoolTest extends TestCase { + private IMAPClientFactory&MockObject $imapClientFactory; private JmapClientFactory&MockObject $jmapClientFactory; private ConnectionPool $pool; protected function setUp(): void { parent::setUp(); + $this->imapClientFactory = $this->createMock(IMAPClientFactory::class); $this->jmapClientFactory = $this->createMock(JmapClientFactory::class); - $this->pool = new ConnectionPool($this->jmapClientFactory); + $this->pool = new ConnectionPool( + $this->imapClientFactory, + $this->jmapClientFactory, + $this->createMock(LoggerInterface::class), + ); } - private function account(int $id): Account { + private function account(int $id, string $protocol = MailAccount::PROTOCOL_JMAP): Account { $mailAccount = new MailAccount(); $mailAccount->setId($id); - $mailAccount->setProtocol(MailAccount::PROTOCOL_JMAP); - $mailAccount->setInboundHost('jmap.example.com'); + $mailAccount->setProtocol($protocol); + $mailAccount->setInboundHost('mail.example.com'); $mailAccount->setInboundPort(443); $mailAccount->setInboundSslMode('yes'); $mailAccount->setInboundUser('user@example.com'); @@ -42,6 +52,24 @@ private function account(int $id): Account { return new Account($mailAccount); } + private function imapAccount(int $id): Account { + return $this->account($id, MailAccount::PROTOCOL_IMAP); + } + + /** + * Counts logouts without an invocation matcher, because clients left in + * the pool are logged out again by its shutdown function. + */ + private function countLogouts(HordeImapClient&MockObject $client): \stdClass { + $counter = new \stdClass(); + $counter->count = 0; + $client->method('logout') + ->willReturnCallback(function () use ($counter): void { + $counter->count++; + }); + return $counter; + } + public function testReusesClientForSameAccount(): void { $account = $this->account(1); $client = $this->createMock(JmapClient::class); @@ -159,4 +187,142 @@ public function testDoesNotPoolFailedCreation(): void { self::assertSame($client, $result); } + + public function testReusesImapClientForSameAccount(): void { + $account = $this->imapAccount(1); + $client = $this->createMock(HordeImapClient::class); + $logouts = $this->countLogouts($client); + $this->imapClientFactory->expects(self::once()) + ->method('getClient') + ->with($account, true) + ->willReturn($client); + + $first = $this->pool->imap($account); + $second = $this->pool->imap($account); + + self::assertSame($client, $first); + self::assertSame($client, $second); + self::assertSame(0, $logouts->count); + } + + public function testKeepsCachedAndUncachedImapClientsApart(): void { + $account = $this->imapAccount(1); + $cached = $this->createMock(HordeImapClient::class); + $uncached = $this->createMock(HordeImapClient::class); + $this->imapClientFactory->expects(self::exactly(2)) + ->method('getClient') + ->willReturnCallback(fn (Account $account, bool $useCache) => $useCache ? $cached : $uncached); + + $resultCached = $this->pool->imap($account); + $resultUncached = $this->pool->imap($account, false); + + self::assertSame($cached, $resultCached); + self::assertSame($uncached, $resultUncached); + self::assertSame($cached, $this->pool->imap($account)); + self::assertSame($uncached, $this->pool->imap($account, false)); + } + + public function testReplacesImapClientAndLogsOutOldOneWhenCredentialsChange(): void { + $account = $this->imapAccount(1); + $oldClient = $this->createMock(HordeImapClient::class); + $oldClient->expects(self::once()) + ->method('logout'); + $newClient = $this->createMock(HordeImapClient::class); + $this->imapClientFactory->expects(self::exactly(2)) + ->method('getClient') + ->willReturnOnConsecutiveCalls($oldClient, $newClient); + $this->pool->imap($account); + + $account->getMailAccount()->setOauthAccessToken('refreshed'); + $result = $this->pool->imap($account); + + self::assertSame($newClient, $result); + } + + public function testKeepsImapClientWhenCreationRefreshedTheToken(): void { + $account = $this->imapAccount(1); + $client = $this->createMock(HordeImapClient::class); + $this->imapClientFactory->expects(self::once()) + ->method('getClient') + ->willReturnCallback(function (Account $account) use ($client) { + $account->getMailAccount()->setOauthAccessToken('refreshed'); + return $client; + }); + $this->pool->imap($account); + + $result = $this->pool->imap($account); + + self::assertSame($client, $result); + } + + public function testResetsLostImapConnectionOnReuse(): void { + $account = $this->imapAccount(1); + $client = $this->createMock(HordeImapClient::class); + $client->method('isConnectionLost') + ->willReturn(true); + $logouts = $this->countLogouts($client); + $this->imapClientFactory->expects(self::once()) + ->method('getClient') + ->willReturn($client); + $this->pool->imap($account); + + $result = $this->pool->imap($account); + + self::assertSame($client, $result); + self::assertSame(1, $logouts->count); + } + + public function testReleaseLogsOutAllImapClientsOfAccount(): void { + $account = $this->imapAccount(1); + $cached = $this->createMock(HordeImapClient::class); + $cached->expects(self::once()) + ->method('logout'); + $uncached = $this->createMock(HordeImapClient::class); + $uncached->expects(self::once()) + ->method('logout'); + $this->imapClientFactory->method('getClient') + ->willReturnCallback(fn (Account $account, bool $useCache) => $useCache ? $cached : $uncached); + $this->pool->imap($account); + $this->pool->imap($account, false); + + $this->pool->release($account); + } + + public function testReleaseIgnoresImapLogoutFailure(): void { + $account = $this->imapAccount(1); + $client = $this->createMock(HordeImapClient::class); + $client->expects(self::once()) + ->method('logout') + ->willThrowException(new Horde_Imap_Client_Exception('Connection reset')); + $newClient = $this->createMock(HordeImapClient::class); + $this->imapClientFactory->expects(self::exactly(2)) + ->method('getClient') + ->willReturnOnConsecutiveCalls($client, $newClient); + $this->pool->imap($account); + + $this->pool->release($account); + + self::assertSame($newClient, $this->pool->imap($account)); + } + + public function testReleaseAllLogsOutEveryImapClient(): void { + $accountA = $this->imapAccount(1); + $accountB = $this->imapAccount(2); + $clientA = $this->createMock(HordeImapClient::class); + $clientA->expects(self::once()) + ->method('logout'); + $clientB = $this->createMock(HordeImapClient::class); + $clientB->expects(self::once()) + ->method('logout'); + $this->imapClientFactory->method('getClient') + ->willReturnCallback(fn (Account $account) => match ($account) { + $accountA => $clientA, + $accountB => $clientB, + }); + $this->pool->imap($accountA); + $this->pool->imap($accountB); + + $this->pool->releaseAll(); + $this->pool->releaseAll(); + } } diff --git a/tests/Unit/Protocol/ProtocolFactoryTest.php b/tests/Unit/Protocol/ProtocolFactoryTest.php index c5e7df5ea7..a85f40eb7d 100644 --- a/tests/Unit/Protocol/ProtocolFactoryTest.php +++ b/tests/Unit/Protocol/ProtocolFactoryTest.php @@ -10,11 +10,13 @@ namespace OCA\Mail\Tests\Unit\Protocol; use ChristophWurst\Nextcloud\Testing\TestCase; +use Horde_Imap_Client_Exception; use JmapClient\Client as JmapClient; use JmapClient\Session\Session; use OCA\Mail\Account; use OCA\Mail\Db\MailAccount; use OCA\Mail\Exception\ServiceException; +use OCA\Mail\IMAP\HordeImapClient; use OCA\Mail\IMAP\IMAPClientFactory; use OCA\Mail\JMAP\JmapClientFactory; use OCA\Mail\Protocol\ConnectionPool; @@ -23,6 +25,7 @@ use Psr\Container\ContainerInterface; class ProtocolFactoryTest extends TestCase { + private IMAPClientFactory&MockObject $imapClientFactory; private JmapClientFactory&MockObject $jmapClientFactory; private ConnectionPool&MockObject $connectionPool; private ProtocolFactory $factory; @@ -30,12 +33,13 @@ class ProtocolFactoryTest extends TestCase { protected function setUp(): void { parent::setUp(); + $this->imapClientFactory = $this->createMock(IMAPClientFactory::class); $this->jmapClientFactory = $this->createMock(JmapClientFactory::class); $this->connectionPool = $this->createMock(ConnectionPool::class); $this->factory = new ProtocolFactory( $this->createMock(ContainerInterface::class), - $this->createMock(IMAPClientFactory::class), + $this->imapClientFactory, $this->jmapClientFactory, $this->connectionPool, ); @@ -48,6 +52,83 @@ private function account(string $protocol): Account { return new Account($mailAccount); } + public function testImapClientComesFromPool(): void { + $account = $this->account(MailAccount::PROTOCOL_IMAP); + $client = $this->createMock(HordeImapClient::class); + $this->connectionPool->expects(self::once()) + ->method('imap') + ->with($account, false) + ->willReturn($client); + $this->imapClientFactory->expects(self::never()) + ->method('getClient'); + + $result = $this->factory->imapClient($account, false); + + self::assertSame($client, $result); + } + + public function testImapClientRejectsJmapAccount(): void { + $account = $this->account(MailAccount::PROTOCOL_JMAP); + $this->connectionPool->expects(self::never()) + ->method('imap'); + $this->expectException(ServiceException::class); + + $this->factory->imapClient($account); + } + + public function testNewImapClientBypassesPool(): void { + $account = $this->account(MailAccount::PROTOCOL_IMAP); + $client = $this->createMock(HordeImapClient::class); + $this->imapClientFactory->expects(self::once()) + ->method('getClient') + ->with($account) + ->willReturn($client); + $this->connectionPool->expects(self::never()) + ->method('imap'); + + $result = $this->factory->newImapClient($account); + + self::assertSame($client, $result); + } + + public function testNewImapClientRejectsJmapAccount(): void { + $account = $this->account(MailAccount::PROTOCOL_JMAP); + $this->imapClientFactory->expects(self::never()) + ->method('getClient'); + $this->expectException(ServiceException::class); + + $this->factory->newImapClient($account); + } + + public function testTestConnectionLogsInWithFreshImapClient(): void { + $account = $this->account(MailAccount::PROTOCOL_IMAP); + $client = $this->createMock(HordeImapClient::class); + $client->expects(self::once()) + ->method('login'); + $client->expects(self::once()) + ->method('logout'); + $this->imapClientFactory->method('getClient') + ->willReturn($client); + $this->connectionPool->expects(self::never()) + ->method('imap'); + + $this->factory->testConnection($account); + } + + public function testTestConnectionLogsOutWhenImapLoginFails(): void { + $account = $this->account(MailAccount::PROTOCOL_IMAP); + $client = $this->createMock(HordeImapClient::class); + $client->method('login') + ->willThrowException(new Horde_Imap_Client_Exception('Authentication failed.')); + $client->expects(self::once()) + ->method('logout'); + $this->imapClientFactory->method('getClient') + ->willReturn($client); + $this->expectException(Horde_Imap_Client_Exception::class); + + $this->factory->testConnection($account); + } + public function testJmapClientComesFromPool(): void { $account = $this->account(MailAccount::PROTOCOL_JMAP); $client = $this->createMock(JmapClient::class); diff --git a/tests/Unit/Service/AntiSpamServiceTest.php b/tests/Unit/Service/AntiSpamServiceTest.php index aeeaf2f23a..a813be08c6 100644 --- a/tests/Unit/Service/AntiSpamServiceTest.php +++ b/tests/Unit/Service/AntiSpamServiceTest.php @@ -174,7 +174,7 @@ public function testSendReportEmail(): void { $this->protocolFactory->expects(self::exactly(2)) ->method('imapClient') ->willReturn($client); - $client->expects(self::exactly(2)) + $client->expects(self::never()) ->method('logout'); $this->imapMessageMapper->expects(self::once()) ->method('getFullText') @@ -240,7 +240,7 @@ public function testSendReportEmailNoSentCopy(): void { $this->protocolFactory->expects(self::exactly(2)) ->method('imapClient') ->willReturn($client); - $client->expects(self::exactly(2)) + $client->expects(self::never()) ->method('logout'); $this->imapMessageMapper->expects(self::once()) ->method('getFullText') diff --git a/tests/Unit/Service/SetupServiceTest.php b/tests/Unit/Service/SetupServiceTest.php index 16b9f9a912..49aa2ea628 100644 --- a/tests/Unit/Service/SetupServiceTest.php +++ b/tests/Unit/Service/SetupServiceTest.php @@ -78,7 +78,7 @@ private function mockSuccessfulImapConnection(): Horde_Imap_Client_Socket&MockOb $imapClient->expects(self::once())->method('logout'); $this->protocolFactory->expects(self::once()) - ->method('imapClient') + ->method('newImapClient') ->willReturn($imapClient); return $imapClient; @@ -209,7 +209,7 @@ public function testCreateNewAccountWithOAuth2(): void { ->method('debug') ->with(self::stringContains('account created ')); - $this->protocolFactory->expects(self::never())->method('imapClient'); + $this->protocolFactory->expects(self::never())->method('newImapClient'); $this->smtpClientFactory->expects(self::never())->method('create'); $this->accountService->expects(self::once()) @@ -273,7 +273,7 @@ public function testCreateNewAccountImapConnectionFailure(): void { ->method('logout'); $this->protocolFactory->expects(self::once()) - ->method('imapClient') + ->method('newImapClient') ->willReturn($imapClient); $this->setupService->createNewAccount( diff --git a/tests/Unit/SetupChecks/MailConnectionPerformanceTest.php b/tests/Unit/SetupChecks/MailConnectionPerformanceTest.php index 9a3fd986dd..47c023fb88 100644 --- a/tests/Unit/SetupChecks/MailConnectionPerformanceTest.php +++ b/tests/Unit/SetupChecks/MailConnectionPerformanceTest.php @@ -117,10 +117,12 @@ public function testConnectionSuccess(): void { ->willReturn([]); $this->protocolFactory->expects($this->once()) - ->method('imapClient') + ->method('newImapClient') ->with(new Account($account)) ->willReturn($client); + $client->expects($this->once()) + ->method('logout'); $this->microtime->method('getNumeric') ->willReturnOnConsecutiveCalls(0, .2, .2); @@ -167,7 +169,7 @@ public function testConnectionWarning(): void { ->willReturn([]); $this->protocolFactory->expects($this->once()) - ->method('imapClient') + ->method('newImapClient') ->with(new Account($account)) ->willReturn($client); @@ -208,7 +210,7 @@ public function testConnectionFailure(): void { ->willReturn($account); $this->protocolFactory->expects($this->once()) - ->method('imapClient') + ->method('newImapClient') ->with(new Account($account)) ->willReturn($client); @@ -245,7 +247,7 @@ public function testClientFailure(): void { ->with(42) ->willReturn($account); $this->protocolFactory->expects($this->once()) - ->method('imapClient') + ->method('newImapClient') ->with(new Account($account)) ->willThrowException(new ServiceException('Something about decryption'));