Skip to content

perf(imap): reuse one IMAP connection per account within a process #13826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SebastianKrupinski wants to merge 1 commit into from
Open

perf(imap): reuse one IMAP connection per account within a process #13826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions lib/BackgroundJob/MigrateImportantJob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}>.");
}
}
}
2 changes: 1 addition & 1 deletion lib/Command/TestAccount.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions lib/IMAP/HordeImapClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 1 addition & 3 deletions lib/IMAP/IMAPClientFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
);
Expand Down
18 changes: 2 additions & 16 deletions lib/IMAP/ImapMailboxConnector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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);
Expand All @@ -77,8 +71,6 @@ public function rename(Account $account, Mailbox $mailbox, string $newName): Mai
$e->getCode(),
$e,
);
} finally {
$client->logout();
}

try {
Expand All @@ -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);
}
Expand All @@ -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());
Expand Down
158 changes: 61 additions & 97 deletions lib/IMAP/ImapMessageConnector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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();
}
}

Expand All @@ -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;
Expand All @@ -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,
);
}

/**
Expand All @@ -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();
}
}

Expand All @@ -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();
}
}

Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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'] ?? [
Expand Down Expand Up @@ -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 {
Expand Down
Loading
Loading