From a23e8238dffd46115c04fd43d835f6db32510023 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 4 Apr 2022 08:39:01 +0200 Subject: [PATCH] Handle IMAP disconnects more explicitly This changes the IMAP client usages to be used as a *resource* that is freed after finished use. Previously we just memoized connections to the same account to lower the number of connections, but that has still shown to cause too many open connections during tests but possibly also in production. Signed-off-by: Christoph Wurst --- lib/Command/DiagnoseAccount.php | 3 +- lib/IMAP/IMAPClientFactory.php | 74 +++--- lib/IMAP/MailboxSync.php | 5 +- lib/IMAP/PreviewEnhancer.php | 5 +- lib/IMAP/Search/Provider.php | 3 +- lib/Listener/DeleteDraftListener.php | 2 + lib/Listener/FlagRepliedMessageListener.php | 4 +- lib/Listener/SaveSentMessageListener.php | 5 +- .../MigrateImportantFromImapAndDb.php | 44 ++-- lib/Service/AutoConfig/ImapConnector.php | 10 +- lib/Service/ItineraryService.php | 25 +- lib/Service/MailManager.php | 106 +++++--- lib/Service/MailTransmission.php | 70 ++++-- lib/Service/SetupService.php | 2 + lib/Service/Sync/ImapToDbSynchronizer.php | 238 +++++++++--------- tests/Integration/Framework/ImapTest.php | 49 ++-- tests/Integration/IMAP/AbstractTest.php | 17 -- tests/Integration/IMAP/MessageMapperTest.php | 69 ++--- 18 files changed, 411 insertions(+), 320 deletions(-) diff --git a/lib/Command/DiagnoseAccount.php b/lib/Command/DiagnoseAccount.php index 94e156b619..e661df9768 100644 --- a/lib/Command/DiagnoseAccount.php +++ b/lib/Command/DiagnoseAccount.php @@ -84,7 +84,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln('No IMAP passwort set. The user might have to log into their account to set it.'); } $imapClient = $this->clientFactory->getClient($account); - try { $this->printCapabilitiesStats($output, $imapClient); $this->printMailboxesMessagesStats($output, $imapClient); @@ -94,6 +93,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int ]); $output->writeln("Horde error occurred: " . $e->getMessage() . ". See nextcloud.log for more details."); return 2; + } finally { + $imapClient->logout(); } return 0; diff --git a/lib/IMAP/IMAPClientFactory.php b/lib/IMAP/IMAPClientFactory.php index 6ef1c7b335..7d53fe13aa 100644 --- a/lib/IMAP/IMAPClientFactory.php +++ b/lib/IMAP/IMAPClientFactory.php @@ -41,8 +41,6 @@ class IMAPClientFactory { /** @var ICacheFactory */ private $cacheFactory; - private $cache = []; - /** * @param ICrypto $crypto * @param IConfig $config @@ -55,47 +53,49 @@ public function __construct(ICrypto $crypto, IConfig $config, ICacheFactory $cac } /** + * Get the connection object for the given account + * + * Connections are not closed until destruction, so the caller site is + * responsible to log out as soon as possible to keep the number of open + * (and stale) connections at a minimum. + * * @param Account $account * @return Horde_Imap_Client_Socket */ public function getClient(Account $account): Horde_Imap_Client_Socket { - if (!isset($this->cache[$account->getId()])) { - $host = $account->getMailAccount()->getInboundHost(); - $user = $account->getMailAccount()->getInboundUser(); - $password = $account->getMailAccount()->getInboundPassword(); - $password = $this->crypto->decrypt($password); - $port = $account->getMailAccount()->getInboundPort(); - $sslMode = $account->getMailAccount()->getInboundSslMode(); - if ($sslMode === 'none') { - $sslMode = false; - } + $host = $account->getMailAccount()->getInboundHost(); + $user = $account->getMailAccount()->getInboundUser(); + $password = $account->getMailAccount()->getInboundPassword(); + $password = $this->crypto->decrypt($password); + $port = $account->getMailAccount()->getInboundPort(); + $sslMode = $account->getMailAccount()->getInboundSslMode(); + if ($sslMode === 'none') { + $sslMode = false; + } - $params = [ - 'username' => $user, - 'password' => $password, - 'hostspec' => $host, - 'port' => $port, - 'secure' => $sslMode, - 'timeout' => (int)$this->config->getSystemValue('app.mail.imap.timeout', 5), - 'context' => [ - 'ssl' => [ - 'verify_peer' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true), - 'verify_peer_name' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true), - ], + $params = [ + 'username' => $user, + 'password' => $password, + 'hostspec' => $host, + 'port' => $port, + 'secure' => $sslMode, + 'timeout' => (int)$this->config->getSystemValue('app.mail.imap.timeout', 5), + 'context' => [ + 'ssl' => [ + 'verify_peer' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true), + 'verify_peer_name' => $this->config->getSystemValueBool('app.mail.verify-tls-peer', true), ], - ]; - if ($this->cacheFactory->isAvailable()) { - $params['cache'] = [ - 'backend' => new Cache([ - 'cacheob' => $this->cacheFactory->createDistributed(md5((string)$account->getId())), - ])]; - } - if ($this->config->getSystemValue('debug', false)) { - $params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log'; - } - $this->cache[$account->getId()] = new Horde_Imap_Client_Socket($params); + ], + ]; + if ($this->cacheFactory->isAvailable()) { + $params['cache'] = [ + 'backend' => new Cache([ + 'cacheob' => $this->cacheFactory->createDistributed(md5((string)$account->getId())), + ])]; } - - return $this->cache[$account->getId()]; + if ($this->config->getSystemValue('debug', false)) { + $params['debug'] = $this->config->getSystemValue('datadirectory') . '/horde_imap.log'; + } + return new Horde_Imap_Client_Socket($params); } } diff --git a/lib/IMAP/MailboxSync.php b/lib/IMAP/MailboxSync.php index dc61307be8..7960c7a632 100644 --- a/lib/IMAP/MailboxSync.php +++ b/lib/IMAP/MailboxSync.php @@ -98,6 +98,8 @@ public function sync(Account $account, ); } catch (Horde_Imap_Client_Exception $e) { $logger->debug('Getting namespaces for account ' . $account->getId() . ' failed: ' . $e->getMessage()); + } finally { + $client->logout(); } try { @@ -137,7 +139,6 @@ 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()); } catch (Horde_Imap_Client_Exception $e) { @@ -147,6 +148,8 @@ public function syncStats(Account $account, Mailbox $mailbox): void { (int)$e->getCode(), $e ); + } finally { + $client->logout(); } $mailbox->setMessages($stats->getTotal()); diff --git a/lib/IMAP/PreviewEnhancer.php b/lib/IMAP/PreviewEnhancer.php index 1f20bdf32e..c07d65d76c 100644 --- a/lib/IMAP/PreviewEnhancer.php +++ b/lib/IMAP/PreviewEnhancer.php @@ -81,9 +81,10 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar return $messages; } + $client = $this->clientFactory->getClient($account); try { $data = $this->imapMapper->getBodyStructureData( - $this->clientFactory->getClient($account), + $client, $mailbox->getName(), $needAnalyze ); @@ -94,6 +95,8 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar ]); return $messages; + } finally { + $client->logout(); } return $this->mapper->updatePreviewDataBulk(...array_map(function (Message $message) use ($data) { diff --git a/lib/IMAP/Search/Provider.php b/lib/IMAP/Search/Provider.php index 8f168f2115..ab403698a7 100644 --- a/lib/IMAP/Search/Provider.php +++ b/lib/IMAP/Search/Provider.php @@ -51,7 +51,6 @@ public function findMatches(Account $account, Mailbox $mailbox, SearchQuery $searchQuery): array { $client = $this->clientFactory->getClient($account); - try { $fetchResult = $client->search( $mailbox->getName(), @@ -59,6 +58,8 @@ public function findMatches(Account $account, ); } 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; diff --git a/lib/Listener/DeleteDraftListener.php b/lib/Listener/DeleteDraftListener.php index 8d89d229b4..61ffe386bf 100644 --- a/lib/Listener/DeleteDraftListener.php +++ b/lib/Listener/DeleteDraftListener.php @@ -90,6 +90,8 @@ private function deleteDraft(Account $account, Message $draft): void { } catch (DoesNotExistException $e) { $this->logger->warning("Account has no draft mailbox set, can't delete the draft"); return; + } finally { + $client->logout(); } try { diff --git a/lib/Listener/FlagRepliedMessageListener.php b/lib/Listener/FlagRepliedMessageListener.php index 977b95fafa..367ec1f7ea 100644 --- a/lib/Listener/FlagRepliedMessageListener.php +++ b/lib/Listener/FlagRepliedMessageListener.php @@ -78,8 +78,8 @@ public function handle(Event $event): void { return; } + $client = $this->imapClientFactory->getClient($event->getAccount()); try { - $client = $this->imapClientFactory->getClient($event->getAccount()); $this->messageMapper->addFlag( $client, $mailbox, @@ -90,6 +90,8 @@ public function handle(Event $event): void { $this->logger->warning('Could not flag replied message: ' . $e, [ 'exception' => $e, ]); + } finally { + $client->logout(); } } } diff --git a/lib/Listener/SaveSentMessageListener.php b/lib/Listener/SaveSentMessageListener.php index 37eedd93c9..307cc939a2 100644 --- a/lib/Listener/SaveSentMessageListener.php +++ b/lib/Listener/SaveSentMessageListener.php @@ -83,14 +83,17 @@ public function handle(Event $event): void { return; } + $client = $this->imapClientFactory->getClient($event->getAccount()); try { $this->messageMapper->save( - $this->imapClientFactory->getClient($event->getAccount()), + $client, $sentMailbox, $event->getMail() ); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException('Could not save sent message on IMAP', 0, $e); + } finally { + $client->logout(); } } } diff --git a/lib/Migration/MigrateImportantFromImapAndDb.php b/lib/Migration/MigrateImportantFromImapAndDb.php index 17f13e912c..f6e693d5fc 100644 --- a/lib/Migration/MigrateImportantFromImapAndDb.php +++ b/lib/Migration/MigrateImportantFromImapAndDb.php @@ -65,34 +65,42 @@ public function __construct(IMAPClientFactory $clientFactory, public function migrateImportantOnImap(Account $account, Mailbox $mailbox): void { $client = $this->clientFactory->getClient($account); - //get all messages that have an $important label from IMAP try { - $uids = $this->messageMapper->getFlagged($client, $mailbox, '$important'); - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException("Could not fetch UIDs of important messages: " . $e->getMessage(), 0, $e); - } - // add $label1 for all that are tagged on IMAP - if (!empty($uids)) { + //get all messages that have an $important label from IMAP try { - $this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT); + $uids = $this->messageMapper->getFlagged($client, $mailbox, '$important'); } catch (Horde_Imap_Client_Exception $e) { - $this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>'); - throw new ServiceException($e->getMessage(), 0, $e); + throw new ServiceException("Could not fetch UIDs of important messages: " . $e->getMessage(), 0, $e); + } + // add $label1 for all that are tagged on IMAP + if (!empty($uids)) { + try { + $this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT); + } catch (Horde_Imap_Client_Exception $e) { + $this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>'); + throw new ServiceException($e->getMessage(), 0, $e); + } } + } finally { + $client->logout(); } } public function migrateImportantFromDb(Account $account, Mailbox $mailbox): void { $client = $this->clientFactory->getClient($account); - $uids = $this->mailboxMapper->findFlaggedImportantUids($mailbox->getId()); - // store our data on imap - if (!empty($uids)) { - try { - $this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT); - } catch (Horde_Imap_Client_Exception $e) { - $this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>'); - throw new ServiceException($e->getMessage(), 0, $e); + try { + $uids = $this->mailboxMapper->findFlaggedImportantUids($mailbox->getId()); + // store our data on imap + if (!empty($uids)) { + try { + $this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT); + } catch (Horde_Imap_Client_Exception $e) { + $this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>'); + throw new ServiceException($e->getMessage(), 0, $e); + } } + } finally { + $client->logout(); } } } diff --git a/lib/Service/AutoConfig/ImapConnector.php b/lib/Service/AutoConfig/ImapConnector.php index 81478427f1..6407f338f0 100644 --- a/lib/Service/AutoConfig/ImapConnector.php +++ b/lib/Service/AutoConfig/ImapConnector.php @@ -85,8 +85,12 @@ public function connect(string $email, $a = new Account($account); $client = $this->clientFactory->getClient($a); - $client->login(); - $this->logger->info("Test-Account-Successful: $this->userId, $host, $port, $user, $encryptionProtocol"); - return $account; + try { + $client->login(); + $this->logger->info("Test-Account-Successful: $this->userId, $host, $port, $user, $encryptionProtocol"); + return $account; + } finally { + $client->logout(); + } } } diff --git a/lib/Service/ItineraryService.php b/lib/Service/ItineraryService.php index a4c0ee7bd9..3f28f106ea 100644 --- a/lib/Service/ItineraryService.php +++ b/lib/Service/ItineraryService.php @@ -81,18 +81,21 @@ public function extract(Account $account, string $mailbox, int $id): Itinerary { } $client = $this->clientFactory->getClient($account); - - $itinerary = new Itinerary(); - $htmlBody = $this->messageMapper->getHtmlBody($client, $mailbox->getName(), $id); - if ($htmlBody !== null) { - $itinerary = $itinerary->merge( - $this->extractor->extract($htmlBody) - ); - $this->logger->debug('Extracted ' . count($itinerary) . ' itinerary entries from the message HTML body'); - } else { - $this->logger->debug('Message does not have an HTML body, can\'t extract itinerary info'); + try { + $itinerary = new Itinerary(); + $htmlBody = $this->messageMapper->getHtmlBody($client, $mailbox->getName(), $id); + if ($htmlBody !== null) { + $itinerary = $itinerary->merge( + $this->extractor->extract($htmlBody) + ); + $this->logger->debug('Extracted ' . count($itinerary) . ' itinerary entries from the message HTML body'); + } else { + $this->logger->debug('Message does not have an HTML body, can\'t extract itinerary info'); + } + $attachments = $this->messageMapper->getRawAttachments($client, $mailbox->getName(), $id); + } finally { + $client->logout(); } - $attachments = $this->messageMapper->getRawAttachments($client, $mailbox->getName(), $id); $itinerary = array_reduce($attachments, function (Itinerary $combined, string $attachment) { $extracted = $this->extractor->extract($attachment); $this->logger->debug('Extracted ' . count($extracted) . ' itinerary entries from an attachment'); diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php index 6ad1244875..1776cf6701 100644 --- a/lib/Service/MailManager.php +++ b/lib/Service/MailManager.php @@ -150,9 +150,8 @@ public function getMailboxes(Account $account): array { */ public function createMailbox(Account $account, string $name): Mailbox { $client = $this->imapClientFactory->getClient($account); - - $folder = $this->folderMapper->createFolder($client, $account, $name); try { + $folder = $this->folderMapper->createFolder($client, $account, $name); $this->folderMapper->getFoldersStatus([$folder], $client); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException( @@ -161,6 +160,8 @@ public function createMailbox(Account $account, string $name): Mailbox { (int)$e->getCode(), $e ); + } finally { + $client->logout(); } $this->folderMapper->detectFolderSpecialUse([$folder]); @@ -174,7 +175,6 @@ public function getImapMessage(Account $account, int $uid, bool $loadBody = false): IMAPMessage { $client = $this->imapClientFactory->getClient($account); - try { return $this->imapMessageMapper->find( $client, @@ -188,6 +188,8 @@ public function getImapMessage(Account $account, (int)$e->getCode(), $e ); + } finally { + $client->logout(); } } @@ -215,7 +217,6 @@ public function getMessage(string $uid, int $id): Message { */ public function getSource(Account $account, string $mailbox, int $uid): ?string { $client = $this->imapClientFactory->getClient($account); - try { return $this->imapMessageMapper->getFullText( $client, @@ -224,6 +225,8 @@ public function getSource(Account $account, string $mailbox, int $uid): ?string ); } catch (Horde_Imap_Client_Exception | DoesNotExistException $e) { throw new ServiceException("Could not load message", 0, $e); + } finally { + $client->logout(); } } @@ -295,20 +298,25 @@ public function deleteMessage(Account $account, throw new ServiceException("No trash folder", 0, $e); } - if ($mailboxId === $trashMailbox->getName()) { - // Delete inside trash -> expunge - $this->imapMessageMapper->expunge( - $this->imapClientFactory->getClient($account), - $sourceMailbox->getName(), - $messageId - ); - } else { - $this->imapMessageMapper->move( - $this->imapClientFactory->getClient($account), - $sourceMailbox->getName(), - $messageId, - $trashMailbox->getName() - ); + $client = $this->imapClientFactory->getClient($account); + try { + if ($mailboxId === $trashMailbox->getName()) { + // Delete inside trash -> expunge + $this->imapMessageMapper->expunge( + $client, + $sourceMailbox->getName(), + $messageId + ); + } else { + $this->imapMessageMapper->move( + $client, + $sourceMailbox->getName(), + $messageId, + $trashMailbox->getName() + ); + } + } finally { + $client->logout(); } $this->eventDispatcher->dispatch( @@ -332,14 +340,20 @@ private function moveMessageOnSameAccount(Account $account, string $destFolderId, int $messageId): void { $client = $this->imapClientFactory->getClient($account); - - $this->imapMessageMapper->move($client, $sourceFolderId, $messageId, $destFolderId); + try { + $this->imapMessageMapper->move($client, $sourceFolderId, $messageId, $destFolderId); + } finally { + $client->logout(); + } } public function markFolderAsRead(Account $account, Mailbox $mailbox): void { $client = $this->imapClientFactory->getClient($account); - - $this->imapMessageMapper->markAllRead($client, $mailbox->getName()); + try { + $this->imapMessageMapper->markAllRead($client, $mailbox->getName()); + } finally { + $client->logout(); + } } public function updateSubscription(Account $account, Mailbox $mailbox, bool $subscribed): Mailbox { @@ -355,6 +369,8 @@ public function updateSubscription(Account $account, Mailbox $mailbox, bool $sub (int)$e->getCode(), $e ); + } finally { + $client->logout(); } /** @@ -376,16 +392,16 @@ public function enableMailboxBackgroundSync(Mailbox $mailbox, } public function flagMessage(Account $account, string $mailbox, int $uid, string $flag, bool $value): void { - $client = $this->imapClientFactory->getClient($account); try { $mb = $this->mailboxMapper->find($account, $mailbox); } catch (DoesNotExistException $e) { throw new ClientException("Mailbox $mailbox does not exist", 0, $e); } - // Only send system flags to the IMAP server as other flags might not be supported - $imapFlags = $this->filterFlags($account, $flag, $mailbox); + $client = $this->imapClientFactory->getClient($account); try { + // Only send system flags to the IMAP server as other flags might not be supported + $imapFlags = $this->filterFlags($account, $flag, $mailbox); foreach ($imapFlags as $imapFlag) { if (empty($imapFlag) === true) { continue; @@ -402,6 +418,8 @@ public function flagMessage(Account $account, string $mailbox, int $uid, string (int)$e->getCode(), $e ); + } finally { + $client->logout(); } $this->eventDispatcher->dispatch( @@ -433,13 +451,13 @@ public function flagMessage(Account $account, string $mailbox, int $uid, string * @link https://github.com/nextcloud/mail/issues/25 */ public function tagMessage(Account $account, string $mailbox, Message $message, Tag $tag, bool $value): void { - $client = $this->imapClientFactory->getClient($account); try { $mb = $this->mailboxMapper->find($account, $mailbox); } catch (DoesNotExistException $e) { throw new ClientException("Mailbox $mailbox does not exist", 0, $e); } if ($this->isPermflagsEnabled($account, $mailbox) === true) { + $client = $this->imapClientFactory->getClient($account); try { if ($value) { // imap keywords and flags work the same way @@ -453,6 +471,8 @@ public function tagMessage(Account $account, string $mailbox, Message $message, (int)$e->getCode(), $e ); + } finally { + $client->logout(); } } if ($value) { @@ -469,17 +489,19 @@ public function tagMessage(Account $account, string $mailbox, Message $message, * @see https://tools.ietf.org/html/rfc2087 */ public function getQuota(Account $account): ?Quota { - $client = $this->imapClientFactory->getClient($account); /** * Get all the quotas roots of the user's mailboxes */ + $client = $this->imapClientFactory->getClient($account); try { $quotas = array_map(static function (Folder $mb) use ($client) { return $client->getQuotaRoot($mb->getMailbox()); }, $this->folderMapper->getFolders($account, $client)); } catch (Horde_Imap_Client_Exception_NoSupportExtension $ex) { return null; + } finally { + $client->logout(); } /** @@ -516,11 +538,16 @@ public function renameMailbox(Account $account, Mailbox $mailbox, string $name): /* * 1. Rename on IMAP */ - $this->folderMapper->renameFolder( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $name - ); + $client = $this->imapClientFactory->getClient($account); + try { + $this->folderMapper->renameFolder( + $client, + $mailbox->getName(), + $name + ); + } finally { + $client->logout(); + } /** * 2. Get the IMAP changes into our database cache @@ -546,7 +573,11 @@ public function renameMailbox(Account $account, Mailbox $mailbox, string $name): public function deleteMailbox(Account $account, Mailbox $mailbox): void { $client = $this->imapClientFactory->getClient($account); - $this->folderMapper->delete($client, $mailbox->getName()); + try { + $this->folderMapper->delete($client, $mailbox->getName()); + } finally { + $client->logout(); + } $this->mailboxMapper->delete($mailbox); } @@ -557,7 +588,12 @@ public function deleteMailbox(Account $account, * @return array[] */ public function getMailAttachments(Account $account, Mailbox $mailbox, Message $message): array { - return $this->imapMessageMapper->getAttachments($this->imapClientFactory->getClient($account), $mailbox->getName(), $message->getUid()); + $client = $this->imapClientFactory->getClient($account); + try { + return $this->imapMessageMapper->getAttachments($client, $mailbox->getName(), $message->getUid()); + } finally { + $client->logout(); + } } /** @@ -612,6 +648,8 @@ public function isPermflagsEnabled(Account $account, string $mailbox): bool { (int)$e->getCode(), $e ); + } finally { + $client->logout(); } return (is_array($capabilities) === true && array_key_exists('permflags', $capabilities) === true && in_array("\*", $capabilities['permflags'], true) === true); } diff --git a/lib/Service/MailTransmission.php b/lib/Service/MailTransmission.php index 23fe8bdda4..33aefce7b0 100644 --- a/lib/Service/MailTransmission.php +++ b/lib/Service/MailTransmission.php @@ -281,12 +281,12 @@ public function saveDraft(NewMessageData $message, Message $previousDraft = null $perfLogger->step('build draft message'); // 'Send' the message + $client = $this->imapClientFactory->getClient($account); try { $transport = new Horde_Mail_Transport_Null(); $mail->send($transport, false, false); $perfLogger->step('create IMAP message'); // save the message in the drafts folder - $client = $this->imapClientFactory->getClient($account); $draftsMailboxId = $account->getMailAccount()->getDraftsMailboxId(); if ($draftsMailboxId === null) { throw new ClientException("No drafts mailbox configured"); @@ -303,6 +303,8 @@ public function saveDraft(NewMessageData $message, Message $previousDraft = null throw new ServiceException('Drafts mailbox does not exist', 0, $e); } catch (Horde_Exception $e) { throw new ServiceException('Could not save draft message', 0, $e); + } finally { + $client->logout(); } $this->eventDispatcher->dispatch( @@ -404,11 +406,16 @@ private function handleForwardedMessageAttachment(Account $account, array $attac $attachmentMessage = $this->mailManager->getMessage($userId, (int)$attachment['id']); $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); - $fullText = $this->messageMapper->getFullText( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $attachmentMessage->getUid() - ); + $client = $this->imapClientFactory->getClient($account); + try { + $fullText = $this->messageMapper->getFullText( + $client, + $mailbox->getName(), + $attachmentMessage->getUid() + ); + } finally { + $client->logout(); + } $message->addRawAttachment( $attachment['displayName'] ?? $attachmentMessage->getSubject() . '.eml', @@ -429,11 +436,16 @@ private function handleEmbeddedMessageAttachments(Account $account, array $attac $attachmentMessage = $this->mailManager->getMessage($userId, (int)$attachment['id']); $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); - $fullText = $this->messageMapper->getFullText( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $attachmentMessage->getUid() - ); + $client = $this->imapClientFactory->getClient($account); + try { + $fullText = $this->messageMapper->getFullText( + $client, + $mailbox->getName(), + $attachmentMessage->getUid() + ); + } finally { + $client->logout(); + } $message->addEmbeddedMessageAttachment( $attachment['displayName'] ?? $attachmentMessage->getSubject() . '.eml', @@ -454,14 +466,19 @@ private function handleForwardedAttachment(Account $account, array $attachment, $userId = $account->getMailAccount()->getUserId(); $attachmentMessage = $this->mailManager->getMessage($userId, (int)$attachment['messageId']); $mailbox = $this->mailManager->getMailbox($userId, $attachmentMessage->getMailboxId()); - $attachments = $this->messageMapper->getRawAttachments( - $this->imapClientFactory->getClient($account), - $mailbox->getName(), - $attachmentMessage->getUid(), - [ - $attachment['id'] - ] - ); + $client = $this->imapClientFactory->getClient($account); + try { + $attachments = $this->messageMapper->getRawAttachments( + $client, + $mailbox->getName(), + $attachmentMessage->getUid(), + [ + $attachment['id'] + ] + ); + } finally { + $client->logout(); + } // Attaches attachment to new message $message->addRawAttachment($attachment['fileName'], $attachments[0]); @@ -495,8 +512,6 @@ private function handleCloudAttachment(array $attachment, IMessage $message) { } public function sendMdn(Account $account, Mailbox $mailbox, Message $message): void { - $imapClient = $this->imapClientFactory->getClient($account); - $query = new Horde_Imap_Client_Fetch_Query(); $query->flags(); $query->uid(); @@ -506,10 +521,15 @@ public function sendMdn(Account $account, Mailbox $mailbox, Message $message): v 'peek' => true, ]); - /** @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); + $imapClient = $this->imapClientFactory->getClient($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(); + } if (count($fetchResults) < 1) { throw new ServiceException('Message "' .$message->getId() . '" not found.'); diff --git a/lib/Service/SetupService.php b/lib/Service/SetupService.php index 89152eb96f..9ab0b1374d 100644 --- a/lib/Service/SetupService.php +++ b/lib/Service/SetupService.php @@ -167,6 +167,8 @@ protected function testConnectivity(Account $account): void { $imapClient->login(); } catch (Horde_Imap_Client_Exception $e) { throw new CouldNotConnectException($e, 'IMAP', $mailAccount->getInboundHost(), $mailAccount->getInboundPort()); + } finally { + $imapClient->logout(); } $transport = $this->smtpClientFactory->create($account); diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index ea000c4067..ef24441e4a 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -287,37 +287,41 @@ private function runInitialSync(Account $account, $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); $client = $this->clientFactory->getClient($account); try { - $imapMessages = $this->imapMapper->findAll( - $client, - $mailbox->getName(), - self::MAX_NEW_MESSAGES, - $highestKnownUid ?? 0 - ); - $perf->step('fetch all messages from IMAP'); - } catch (Horde_Imap_Client_Exception $e) { - throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); - } + try { + $imapMessages = $this->imapMapper->findAll( + $client, + $mailbox->getName(), + self::MAX_NEW_MESSAGES, + $highestKnownUid ?? 0 + ); + $perf->step('fetch all messages from IMAP'); + } 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) { - $this->dbMapper->insertBulk($account, ...array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { - return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); - }, $chunk)); - } - $perf->step('persist messages in database'); + foreach (array_chunk($imapMessages['messages'], 500) as $chunk) { + $this->dbMapper->insertBulk($account, ...array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { + return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); + }, $chunk)); + } + $perf->step('persist messages in database'); - if (!$imapMessages['all']) { - // We might need more attempts to fill the cache - $perf->end(); + if (!$imapMessages['all']) { + // We might need more attempts to fill the cache + $perf->end(); - $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); - $total = $imapMessages['total']; - $cached = count($this->messageMapper->findAllUids($mailbox)); - throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); - } + $loggingMailboxId = $account->getId() . ':' . $mailbox->getName(); + $total = $imapMessages['total']; + $cached = count($this->messageMapper->findAllUids($mailbox)); + throw new IncompleteSyncException("Initial sync is not complete for $loggingMailboxId ($cached of $total messages cached)."); + } - $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); - $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); - $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); + $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); + $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + } finally { + $client->logout(); + } $this->mailboxMapper->update($mailbox); $perf->end(); @@ -340,104 +344,108 @@ private function runPartialSync(Account $account, ); $client = $this->clientFactory->getClient($account); - $uids = $knownUids ?? $this->dbMapper->findAllUids($mailbox); - $perf->step('get all known UIDs'); - - if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { - $response = $this->synchronizer->sync( - $client, - new Request( - $mailbox->getName(), - $mailbox->getSyncNewToken(), - $uids - ), - Horde_Imap_Client::SYNC_NEWMSGSUIDS - ); - $perf->step('get new messages via Horde'); + try { + $uids = $knownUids ?? $this->dbMapper->findAllUids($mailbox); + $perf->step('get all known UIDs'); + + if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { + $response = $this->synchronizer->sync( + $client, + new Request( + $mailbox->getName(), + $mailbox->getSyncNewToken(), + $uids + ), + Horde_Imap_Client::SYNC_NEWMSGSUIDS + ); + $perf->step('get new messages via Horde'); + + $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); + if ($highestKnownUid === null) { + // Everything is relevant + $newMessages = $response->getNewMessages(); + } else { + // Filter out anything that is already in the DB. Ideally this never happens, but if there is an error + // during a consecutive chunk INSERT, the sync token won't be updated. In that case the same message(s) + // will be seen as *new* and therefore cause conflicts. + $newMessages = array_filter($response->getNewMessages(), function (IMAPMessage $imapMessage) use ($highestKnownUid) { + return $imapMessage->getUid() > $highestKnownUid; + }); + } - $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); - if ($highestKnownUid === null) { - // Everything is relevant - $newMessages = $response->getNewMessages(); - } else { - // Filter out anything that is already in the DB. Ideally this never happens, but if there is an error - // during a consecutive chunk INSERT, the sync token won't be updated. In that case the same message(s) - // will be seen as *new* and therefore cause conflicts. - $newMessages = array_filter($response->getNewMessages(), function (IMAPMessage $imapMessage) use ($highestKnownUid) { - return $imapMessage->getUid() > $highestKnownUid; - }); - } + foreach (array_chunk($newMessages, 500) as $chunk) { + $dbMessages = array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { + return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); + }, $chunk); - foreach (array_chunk($newMessages, 500) as $chunk) { - $dbMessages = array_map(function (IMAPMessage $imapMessage) use ($mailbox, $account) { - return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); - }, $chunk); + $this->dbMapper->insertBulk($account, ...$dbMessages); - $this->dbMapper->insertBulk($account, ...$dbMessages); + $this->dispatcher->dispatch( + NewMessagesSynchronized::class, + new NewMessagesSynchronized($account, $mailbox, $dbMessages) + ); + $perf->step('classified a chunk of new messages'); + } + $perf->step('persist new messages'); - $this->dispatcher->dispatch( - NewMessagesSynchronized::class, - new NewMessagesSynchronized($account, $mailbox, $dbMessages) - ); - $perf->step('classified a chunk of new messages'); + $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); } - $perf->step('persist new messages'); - - $mailbox->setSyncNewToken($client->getSyncToken($mailbox->getName())); - } - if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) { - $response = $this->synchronizer->sync( - $client, - new Request( - $mailbox->getName(), - $mailbox->getSyncChangedToken(), - $uids - ), - Horde_Imap_Client::SYNC_FLAGSUIDS - ); - $perf->step('get changed messages via Horde'); + if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) { + $response = $this->synchronizer->sync( + $client, + new Request( + $mailbox->getName(), + $mailbox->getSyncChangedToken(), + $uids + ), + Horde_Imap_Client::SYNC_FLAGSUIDS + ); + $perf->step('get changed messages via Horde'); - $permflagsEnabled = $this->mailManager->isPermflagsEnabled($account, $mailbox->getName()); + $permflagsEnabled = $this->mailManager->isPermflagsEnabled($account, $mailbox->getName()); - foreach (array_chunk($response->getChangedMessages(), 500) as $chunk) { - $this->dbMapper->updateBulk($account, $permflagsEnabled, ...array_map(static function (IMAPMessage $imapMessage) use ($mailbox, $account) { - return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); - }, $chunk)); - } - $perf->step('persist changed messages'); - - // If a list of UIDs was *provided* (as opposed to loaded from the DB, - // we can not assume that all changes were detected, hence this is kinda - // a silent sync and we don't update the change token until the next full - // mailbox sync - if ($knownUids === null) { - $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); + foreach (array_chunk($response->getChangedMessages(), 500) as $chunk) { + $this->dbMapper->updateBulk($account, $permflagsEnabled, ...array_map(static function (IMAPMessage $imapMessage) use ($mailbox, $account) { + return $imapMessage->toDbMessage($mailbox->getId(), $account->getMailAccount()); + }, $chunk)); + } + $perf->step('persist changed messages'); + + // If a list of UIDs was *provided* (as opposed to loaded from the DB, + // we can not assume that all changes were detected, hence this is kinda + // a silent sync and we don't update the change token until the next full + // mailbox sync + if ($knownUids === null) { + $mailbox->setSyncChangedToken($client->getSyncToken($mailbox->getName())); + } } - } - if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) { - $response = $this->synchronizer->sync( - $client, - new Request( - $mailbox->getName(), - $mailbox->getSyncVanishedToken(), - $uids - ), - Horde_Imap_Client::SYNC_VANISHEDUIDS - ); - $perf->step('get vanished messages via Horde'); + if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) { + $response = $this->synchronizer->sync( + $client, + new Request( + $mailbox->getName(), + $mailbox->getSyncVanishedToken(), + $uids + ), + Horde_Imap_Client::SYNC_VANISHEDUIDS + ); + $perf->step('get vanished messages via Horde'); - foreach (array_chunk($response->getVanishedMessageUids(), 500) as $chunk) { - $this->dbMapper->deleteByUid($mailbox, ...$chunk); - } - $perf->step('persist new messages'); - - // If a list of UIDs was *provided* (as opposed to loaded from the DB, - // we can not assume that all changes were detected, hence this is kinda - // a silent sync and we don't update the vanish token until the next full - // mailbox sync - if ($knownUids === null) { - $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + foreach (array_chunk($response->getVanishedMessageUids(), 500) as $chunk) { + $this->dbMapper->deleteByUid($mailbox, ...$chunk); + } + $perf->step('persist new messages'); + + // If a list of UIDs was *provided* (as opposed to loaded from the DB, + // we can not assume that all changes were detected, hence this is kinda + // a silent sync and we don't update the vanish token until the next full + // mailbox sync + if ($knownUids === null) { + $mailbox->setSyncVanishedToken($client->getSyncToken($mailbox->getName())); + } } + } finally { + $client->logout(); } $this->mailboxMapper->update($mailbox); $perf->end(); diff --git a/tests/Integration/Framework/ImapTest.php b/tests/Integration/Framework/ImapTest.php index 1a68ad4157..da18d3d1d0 100644 --- a/tests/Integration/Framework/ImapTest.php +++ b/tests/Integration/Framework/ImapTest.php @@ -52,7 +52,7 @@ trait ImapTest { /** * @return Horde_Imap_Client_Socket */ - private function getTestClient() { + private function getTestClient(): Horde_Imap_Client_Socket { if ($this->client === null) { $this->client = new Horde_Imap_Client_Socket([ 'username' => 'user@domain.tld', @@ -134,8 +134,6 @@ public function getMessageBuilder() { * @return int id of the new message */ public function saveMessage(string $mailbox, SimpleMessage $message, MailAccount $account = null) { - $client = $this->getClient($account); - $headers = [ 'From' => new Horde_Mail_Rfc822_Address($message->getFrom()), 'To' => new Horde_Mail_Rfc822_Address($message->getTo()), @@ -155,32 +153,43 @@ public function saveMessage(string $mailbox, SimpleMessage $message, MailAccount $raw = $mail->getRaw(); $data = stream_get_contents($raw); - return $client->append($mailbox, [ - [ - 'data' => $data, - ] - ])->ids[0]; + $client = $this->getClient($account); + try { + return $client->append($mailbox, [ + [ + 'data' => $data, + ] + ])->ids[0]; + } finally { + $client->logout(); + } } public function flagMessage($mailbox, $id, MailAccount $account = null) { $client = $this->getClient($account); - - $client->store($mailbox, [ - 'ids' => new Horde_Imap_Client_Ids([$id]), - 'add' => [ - Horde_Imap_Client::FLAG_FLAGGED, - ], - ]); + try { + $client->store($mailbox, [ + 'ids' => new Horde_Imap_Client_Ids([$id]), + 'add' => [ + Horde_Imap_Client::FLAG_FLAGGED, + ], + ]); + } finally { + $client->logout(); + } } public function deleteMessage($mailbox, $id, MailAccount $account = null) { $client = $this->getClient($account); - $ids = new Horde_Imap_Client_Ids([$id]); - $client->expunge($mailbox, [ - 'ids' => $ids, - 'delete' => true, - ]); + try { + $client->expunge($mailbox, [ + 'ids' => $ids, + 'delete' => true, + ]); + } finally { + $client->logout(); + } } /** diff --git a/tests/Integration/IMAP/AbstractTest.php b/tests/Integration/IMAP/AbstractTest.php index bbaaddf46a..a5119efc5e 100644 --- a/tests/Integration/IMAP/AbstractTest.php +++ b/tests/Integration/IMAP/AbstractTest.php @@ -77,23 +77,6 @@ public static function tearDownAfterClass(): void { } } - /** - * @param string $name - * @return Mailbox - */ - public function createMailBox($name) { - try { - $this->getTestAccount()->getMailbox($name); - $this->deleteMailbox($name); - } catch (Exception $e) { - // Ignore mailbox not found - } - - $mailbox = $this->getTestAccount()->createMailbox($name); - self::$createdMailboxes[$name] = $mailbox; - return $mailbox; - } - /** * @return Account */ diff --git a/tests/Integration/IMAP/MessageMapperTest.php b/tests/Integration/IMAP/MessageMapperTest.php index 20e93e688a..d537bcaa1c 100644 --- a/tests/Integration/IMAP/MessageMapperTest.php +++ b/tests/Integration/IMAP/MessageMapperTest.php @@ -77,10 +77,13 @@ public function testTagging(): void { $newUid = $this->saveMessage($inbox->getName(), $message, $account); // now we tag this message! + $client = $this->getClient($account); try { - $imapMessageMapper->addFlag($this->getClient($account), $mailBox, [$newUid], '$label1'); + $imapMessageMapper->addFlag($client, $mailBox, [$newUid], '$label1'); } catch (Horde_Imap_Client_Exception $e) { self::fail('Could not tag message'); + } finally { + $client->logout(); } // sync @@ -103,10 +106,13 @@ public function testTagging(): void { // now we untag this message! + $client = $this->getClient($account); try { - $imapMessageMapper->removeFlag($this->getClient($account), $mailBox, [$newUid], '$label1'); + $imapMessageMapper->removeFlag($client, $mailBox, [$newUid], '$label1'); } catch (Horde_Imap_Client_Exception $e) { self::fail('Could not untag message'); + } finally { + $client->logout(); } // sync again @@ -151,7 +157,6 @@ public function testGetFlagged(): void { ->finish(); $newUid = $this->saveMessage($inbox->getName(), $message, $account); - // Put another new message into the mailbox $message = $this->getMessageBuilder() ->from('fluffington@domain.tld') @@ -166,38 +171,34 @@ public function testGetFlagged(): void { ->finish(); $this->saveMessage($inbox->getName(), $message, $account); - // now we tag this message with $label1 + $client = $this->getClient($account); try { - $imapMessageMapper->addFlag($this->getClient($account), $mailBox, [$newUid], '$label1'); - } catch (Horde_Imap_Client_Exception $e) { - self::fail('Could not tag message'); + // now we tag this message with $label1 + $imapMessageMapper->addFlag($client, $mailBox, [$newUid], '$label1'); + // now we tag this and the previous message with $label2 + $imapMessageMapper->addFlag($client, $mailBox, [$newUid, $newUid2], '$label2'); + + // test for labels + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, '$label1'); + self::assertNotEmpty($tagged); + // are the counts correct? + self::assertCount(1, $tagged); + + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, '$label2'); + self::assertNotEmpty($tagged); + self::assertCount(2, $tagged); + + // test for labels that wasn't set + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, '$notAvailable'); + self::assertEmpty($tagged); + + // test for regular flag - recent + $tagged = $imapMessageMapper->getFlagged($client, $mailBox, Horde_Imap_Client::FLAG_RECENT); + self::assertNotEmpty($tagged); + // should return all messages + self::assertCount(3, $tagged); + } finally { + $client->logout(); } - - // now we tag this and the previous message with $label2 - try { - $imapMessageMapper->addFlag($this->getClient($account), $mailBox, [$newUid, $newUid2], '$label2'); - } catch (Horde_Imap_Client_Exception $e) { - self::fail('Could not tag message'); - } - - // test for labels - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, '$label1'); - self::assertNotEmpty($tagged); - // are the counts correct? - self::assertCount(1, $tagged); - - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, '$label2'); - self::assertNotEmpty($tagged); - self::assertCount(2, $tagged); - - // test for labels that wasn't set - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, '$notAvailable'); - self::assertEmpty($tagged); - - // test for regular flag - recent - $tagged = $imapMessageMapper->getFlagged($this->getClient($account), $mailBox, Horde_Imap_Client::FLAG_RECENT); - self::assertNotEmpty($tagged); - // should return all messages - self::assertCount(3, $tagged); } }