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 <christoph@winzerhof-wurst.at>

3 files changed, 64 insertions, 71 deletions

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
@@ -78,23 +78,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
 	 */
 	protected function getTestAccount() {
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);
 	}
 }