From a23e8238dffd46115c04fd43d835f6db32510023 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 4 Apr 2022 08:39:01 +0200 Subject: 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 --- tests/Integration/Framework/ImapTest.php | 49 ++++++++++++-------- tests/Integration/IMAP/AbstractTest.php | 17 ------- tests/Integration/IMAP/MessageMapperTest.php | 69 ++++++++++++++-------------- 3 files changed, 64 insertions(+), 71 deletions(-) (limited to 'tests/Integration') diff --git a/tests/Integration/Framework/ImapTest.php b/tests/Integration/Framework/ImapTest.php index 1a68ad415..da18d3d1d 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 @@ trait ImapTest { * @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 @@ trait ImapTest { $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 bbaaddf46..a5119efc5 100644 --- a/tests/Integration/IMAP/AbstractTest.php +++ b/tests/Integration/IMAP/AbstractTest.php @@ -77,23 +77,6 @@ abstract class AbstractTest extends TestCase { } } - /** - * @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 20e93e688..d537bcaa1 100644 --- a/tests/Integration/IMAP/MessageMapperTest.php +++ b/tests/Integration/IMAP/MessageMapperTest.php @@ -77,10 +77,13 @@ class MessageMapperTest extends TestCase { $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 @@ class MessageMapperTest extends TestCase { // 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 @@ class MessageMapperTest extends TestCase { ->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 @@ class MessageMapperTest extends TestCase { ->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); } } -- cgit v1.2.3