bug #35633 [Mailer] Do not ping the SMTP server before sending every …

…message (micheh)

This PR was squashed before being merged into the 4.4 branch (closes #35633).

Discussion
----------

[Mailer] Do not ping the SMTP server before sending every message

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35515
| License       | MIT

This pull request changes the SMTP transport to only ping the server if the last message was sent more than a specified number of seconds ago (instead of pinging the server before every message). By default, it will ping the server if 100 or more seconds since the last message have passed.

This should make sending emails with the SMTP transport more robust with many emails, as SMTP servers will often drop the connection if too many non-mail commands are sent (like pinging the server with NOOP commands).

Commits
-------

2817810 [Mailer] Do not ping the SMTP server before sending every message

src/Symfony/Component/Mailer/Tests/Transport/Smtp/SmtpTransportTest.php

@@ -12,8 +12,12 @@
 namespace Symfony\Component\Mailer\Tests\Transport\Smtp;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Mailer\Envelope;
 use Symfony\Component\Mailer\Transport\Smtp\SmtpTransport;
+use Symfony\Component\Mailer\Transport\Smtp\Stream\AbstractStream;
 use Symfony\Component\Mailer\Transport\Smtp\Stream\SocketStream;
+use Symfony\Component\Mime\Address;
+use Symfony\Component\Mime\RawMessage;
 
 class SmtpTransportTest extends TestCase
 {
@@ -25,4 +29,95 @@ public function testToString()
         $t = new SmtpTransport((new SocketStream())->setHost('127.0.0.1')->setPort(2525)->disableTls());
         $this->assertEquals('smtp://127.0.0.1:2525', (string) $t);
     }
+
+    public function testSendDoesNotPingBelowThreshold(): void
+    {
+        $stream = new DummyStream();
+        $envelope = new Envelope(new Address('sender@example.org'), [new Address('recipient@example.org')]);
+
+        $transport = new SmtpTransport($stream);
+        $transport->send(new RawMessage('Message 1'), $envelope);
+        $transport->send(new RawMessage('Message 2'), $envelope);
+        $transport->send(new RawMessage('Message 3'), $envelope);
+
+        $this->assertNotContains("NOOP\r\n", $stream->getCommands());
+    }
+
+    public function testSendDoesPingAboveThreshold(): void
+    {
+        $stream = new DummyStream();
+        $envelope = new Envelope(new Address('sender@example.org'), [new Address('recipient@example.org')]);
+
+        $transport = new SmtpTransport($stream);
+        $transport->setPingThreshold(1);
+
+        $transport->send(new RawMessage('Message 1'), $envelope);
+        $transport->send(new RawMessage('Message 2'), $envelope);
+
+        $this->assertNotContains("NOOP\r\n", $stream->getCommands());
+
+        $stream->clearCommands();
+        sleep(1);
+
+        $transport->send(new RawMessage('Message 3'), $envelope);
+        $this->assertContains("NOOP\r\n", $stream->getCommands());
+    }
+}
+
+class DummyStream extends AbstractStream
+{
+    /**
+     * @var string
+     */
+    private $nextResponse;
+
+    /**
+     * @var string[]
+     */
+    private $commands;
+
+    public function initialize(): void
+    {
+        $this->nextResponse = '220 localhost';
+    }
+
+    public function write(string $bytes, $debug = true): void
+    {
+        $this->commands[] = $bytes;
+
+        if (0 === strpos($bytes, 'DATA')) {
+            $this->nextResponse = '354 Enter message, ending with "." on a line by itself';
+        } elseif (0 === strpos($bytes, 'QUIT')) {
+            $this->nextResponse = '221 Goodbye';
+        } else {
+            $this->nextResponse = '250 OK';
+        }
+    }
+
+    public function readLine(): string
+    {
+        return $this->nextResponse;
+    }
+
+    public function flush(): void
+    {
+    }
+
+    /**
+     * @return string[]
+     */
+    public function getCommands(): array
+    {
+        return $this->commands;
+    }
+
+    public function clearCommands(): void
+    {
+        $this->commands = [];
+    }
+
+    protected function getReadConnectionDescription(): string
+    {
+        return 'null';
+    }
 }

src/Symfony/Component/Mailer/Transport/Smtp/SmtpTransport.php

@@ -35,6 +35,8 @@ class SmtpTransport extends AbstractTransport
     private $restartThreshold = 100;
     private $restartThresholdSleep = 0;
     private $restartCounter;
+    private $pingThreshold = 100;
+    private $lastMessageTime = 0;
     private $stream;
     private $domain = '[127.0.0.1]';
 
@@ -66,6 +68,28 @@ public function setRestartThreshold(int $threshold, int $sleep = 0): self
         return $this;
     }
 
+    /**
+     * Sets the minimum number of seconds required between two messages, before the server is pinged.
+     * If the transport wants to send a message and the time since the last message exceeds the specified threshold,
+     * the transport will ping the server first (NOOP command) to check if the connection is still alive.
+     * Otherwise the message will be sent without pinging the server first.
+     *
+     * Do not set the threshold too low, as the SMTP server may drop the connection if there are too many
+     * non-mail commands (like pinging the server with NOOP).
+     *
+     * By default, the threshold is set to 100 seconds.
+     *
+     * @param int $seconds The minimum number of seconds between two messages required to ping the server
+     *
+     * @return $this
+     */
+    public function setPingThreshold(int $seconds): self
+    {
+        $this->pingThreshold = $seconds;
+
+        return $this;
+    }
+
     /**
      * Sets the name of the local domain that will be used in HELO.
      *
@@ -160,7 +184,10 @@ public function executeCommand(string $command, array $codes): string
 
     protected function doSend(SentMessage $message): void
     {
-        $this->ping();
+        if (microtime(true) - $this->lastMessageTime > $this->pingThreshold) {
+            $this->ping();
+        }
+
         if (!$this->started) {
             $this->start();
         }
@@ -183,6 +210,8 @@ protected function doSend(SentMessage $message): void
             $e->appendDebug($this->stream->getDebug());
 
             throw $e;
+        } finally {
+            $this->lastMessageTime = microtime(true);
         }
     }
 
@@ -213,6 +242,7 @@ private function start(): void
         $this->assertResponseCode($this->getFullResponse(), [220]);
         $this->doHeloCommand();
         $this->started = true;
+        $this->lastMessageTime = 0;
 
         $this->getLogger()->debug(sprintf('Email transport "%s" started', __CLASS__));
     }