fix: use single connection to sync all mailboxes

Signed-off-by: SebastianKrupinski <[email protected]>

lib/Service/Sync/ImapToDbSynchronizer.php

@@ -109,6 +109,10 @@ public function syncAccount(Account $account,
 		$snoozeMailboxId = $account->getMailAccount()->getSnoozeMailboxId();
 		$sentMailboxId = $account->getMailAccount()->getSentMailboxId();
 		$trashRetentionDays = $account->getMailAccount()->getTrashRetentionDays();
+		// construct imap client and perform initial connection
+		$client = $this->clientFactory->getClient($account);
+		$client->login();
+		// perform mail sync on all mail boxes
 		foreach ($this->mailboxMapper->findAll($account) as $mailbox) {
 			$syncTrash = $trashMailboxId === $mailbox->getId() && $trashRetentionDays !== null;
 			$syncSnooze = $snoozeMailboxId === $mailbox->getId();
@@ -121,6 +125,7 @@ public function syncAccount(Account $account,
 			$logger->debug('Syncing ' . $mailbox->getId());
 			if ($this->sync(
 				$account,
+				$client,
 				$mailbox,
 				$logger,
 				$criteria,
@@ -131,6 +136,8 @@ public function syncAccount(Account $account,
 				$rebuildThreads = true;
 			}
 		}
+		// disconnect imap client
+		$client->logout();
 		$this->dispatcher->dispatchTyped(
 			new SynchronizationEvent(
 				$account,
@@ -192,6 +199,7 @@ private function resetCache(Account $account, Mailbox $mailbox): void {
 	 * @return bool whether to rebuild threads or not
 	 */
 	public function sync(Account $account,
+		Horde_Imap_Client_Base $client,
 		Mailbox $mailbox,
 		LoggerInterface $logger,
 		int $criteria = Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS,
@@ -202,10 +210,6 @@ public function sync(Account $account,
 		if ($mailbox->getSelectable() === false) {
 			return $rebuildThreads;
 		}
-
-		$client = $this->clientFactory->getClient($account);
-		$client->login(); // Need to login before fetching capabilities.
-
 		// There is no partial sync when using QRESYNC. As per RFC the client will always pull
 		// all changes. This is a cheap operation when using QRESYNC as the server keeps track
 		// of a client's state through the sync token. We could just update the sync tokens and
@@ -276,8 +280,6 @@ public function sync(Account $account,
 				$logger->debug('Unlocking mailbox ' . $mailbox->getId() . ' from new messages sync');
 				$this->mailboxMapper->unlockFromNewSync($mailbox);
 			}
-
-			$client->logout();
 		}
 
 		if (!$batchSync) {

lib/Service/Sync/SyncService.php

@@ -19,6 +19,7 @@
 use OCA\Mail\Exception\MailboxLockedException;
 use OCA\Mail\Exception\MailboxNotCachedException;
 use OCA\Mail\Exception\ServiceException;
+use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MailboxSync;
 use OCA\Mail\IMAP\PreviewEnhancer;
 use OCA\Mail\IMAP\Sync\Response;
@@ -29,6 +30,9 @@
 use function array_map;
 
 class SyncService {
+	/** @var IMAPClientFactory */
+	private $clientFactory;
+
 	/** @var ImapToDbSynchronizer */
 	private $synchronizer;
 
@@ -50,13 +54,16 @@ class SyncService {
 	/** @var MailboxSync */
 	private $mailboxSync;
 
-	public function __construct(ImapToDbSynchronizer $synchronizer,
+	public function __construct(
+		IMAPClientFactory $clientFactory,
+		ImapToDbSynchronizer $synchronizer,
 		FilterStringParser $filterStringParser,
 		MailboxMapper $mailboxMapper,
 		MessageMapper $messageMapper,
 		PreviewEnhancer $previewEnhancer,
 		LoggerInterface $logger,
 		MailboxSync $mailboxSync) {
+		$this->clientFactory = $clientFactory;
 		$this->synchronizer = $synchronizer;
 		$this->filterStringParser = $filterStringParser;
 		$this->mailboxMapper = $mailboxMapper;
@@ -103,9 +110,13 @@ public function syncMailbox(Account $account,
 		if ($partialOnly && !$mailbox->isCached()) {
 			throw MailboxNotCachedException::from($mailbox);
 		}
-
+		// construct imap client and perform initial connection
+		$client = $this->clientFactory->getClient($account);
+		$client->login();
+		// perform mail sync on all mail boxes
 		$this->synchronizer->sync(
 			$account,
+			$client,
 			$mailbox,
 			$this->logger,
 			$criteria,
@@ -114,6 +125,8 @@ public function syncMailbox(Account $account,
 		);
 
 		$this->mailboxSync->syncStats($account, $mailbox);
+		// disconnect imap client
+		$client->logout();
 
 		$query = $filter === null ? null : $this->filterStringParser->parse($filter);
 		return $this->getDatabaseSyncChanges(

tests/Unit/Service/Sync/SyncServiceTest.php

@@ -9,11 +9,13 @@
 
 namespace OCA\Mail\Tests\Unit\Service\Sync;
 
+use Horde_Imap_Client_Socket;
 use OCA\Mail\Account;
 use OCA\Mail\Db\Mailbox;
 use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\Db\MessageMapper;
 use OCA\Mail\Exception\MailboxNotCachedException;
+use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MailboxStats;
 use OCA\Mail\IMAP\MailboxSync;
 use OCA\Mail\IMAP\PreviewEnhancer;
@@ -25,6 +27,13 @@
 use Psr\Log\LoggerInterface;
 
 class SyncServiceTest extends TestCase {
+
+	/** @var IMAPClientFactory */
+	private $clientFactory;
+
+	/** @var Horde_Imap_Client_Socket */
+	private $client;
+
 	/** @var ImapToDbSynchronizer */
 	private $synchronizer;
 
@@ -52,6 +61,8 @@ class SyncServiceTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
+		$this->clientFactory = $this->createMock(IMAPClientFactory::class);
+		$this->client = $this->createMock(Horde_Imap_Client_Socket::class);
 		$this->synchronizer = $this->createMock(ImapToDbSynchronizer::class);
 		$this->filterStringParser = $this->createMock(FilterStringParser::class);
 		$this->mailboxMapper = $this->createMock(MailboxMapper::class);
@@ -61,6 +72,7 @@ protected function setUp(): void {
 		$this->mailboxSync = $this->createMock(MailboxSync::class);
 
 		$this->syncService = new SyncService(
+			$this->clientFactory,
 			$this->synchronizer,
 			$this->filterStringParser,
 			$this->mailboxMapper,
@@ -102,7 +114,10 @@ public function testSyncMailboxReturnsFolderStats() {
 			[],
 			new MailboxStats(42, 10, null)
 		);
-
+		$this->clientFactory
+			->method('getClient')
+			->with($account)
+			->willReturn($this->client);
 		$this->messageMapper
 			->method('findUidsForIds')
 			->with($mailbox, [])
@@ -111,6 +126,7 @@ public function testSyncMailboxReturnsFolderStats() {
 			->method('sync')
 			->with(
 				$account,
+				$this->client,
 				$mailbox,
 				$this->loggerInterface,
 				0,