bug symfony#54572 [Mailer] Fix sendmail transport failure handling an…

…d interactive mode (bobvandevijver)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Mailer] Fix sendmail transport failure handling and interactive mode

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix symfony#54532 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

symfony#54239 introduced an issue for us when using sendmail in interactive mode using a long running background worker. It will throw exceptions due to an unclean shutdown in case of an SMTP timeout (5 minute by default), while in interactive mode all output is actually handled by the SMTP transport, including that timeout. That makes the exit code of the long running process not relevant in that mode. See the bug report for some more details.

I have verified that this change solves my production issues, although I am not particularly fond on the hoops I had to jump to show this in the test.

cc `@aboks`

Commits
-------

1b2ead3 [Mailer] Fix sendmail transport failure handling and interactive mode

src/Symfony/Component/Mailer/Tests/Transport/Fixtures/fake-failing-sendmail.php

@@ -1,4 +1,8 @@
 #!/usr/bin/env php
 <?php
+$argsPath = sys_get_temp_dir().\DIRECTORY_SEPARATOR.'sendmail_args';
+
+file_put_contents($argsPath, implode(' ', $argv));
+
 print "Sending failed";
 exit(42);

src/Symfony/Component/Mailer/Tests/Transport/SendmailTransportTest.php

@@ -13,15 +13,21 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Mailer\DelayedEnvelope;
+use Symfony\Component\Mailer\Envelope;
 use Symfony\Component\Mailer\Exception\TransportException;
+use Symfony\Component\Mailer\SentMessage;
 use Symfony\Component\Mailer\Transport\SendmailTransport;
+use Symfony\Component\Mailer\Transport\Smtp\Stream\ProcessStream;
+use Symfony\Component\Mailer\Transport\TransportInterface;
 use Symfony\Component\Mime\Address;
 use Symfony\Component\Mime\Email;
+use Symfony\Component\Mime\RawMessage;
 
 class SendmailTransportTest extends TestCase
 {
     private const FAKE_SENDMAIL = __DIR__.'/Fixtures/fake-sendmail.php -t';
     private const FAKE_FAILING_SENDMAIL = __DIR__.'/Fixtures/fake-failing-sendmail.php -t';
+    private const FAKE_INTERACTIVE_SENDMAIL = __DIR__.'/Fixtures/fake-failing-sendmail.php -bs';
 
     /**
      * @var string
@@ -49,9 +55,7 @@ public function testToString()
 
     public function testToIsUsedWhenRecipientsAreNotSet()
     {
-        if ('\\' === \DIRECTORY_SEPARATOR) {
-            $this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
-        }
+        $this->skipOnWindows();
 
         $mail = new Email();
         $mail
@@ -71,20 +75,9 @@ public function testToIsUsedWhenRecipientsAreNotSet()
 
     public function testRecipientsAreUsedWhenSet()
     {
-        if ('\\' === \DIRECTORY_SEPARATOR) {
-            $this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
-        }
+        $this->skipOnWindows();
 
-        $mail = new Email();
-        $mail
-            ->from('[email protected]')
-            ->to('[email protected]')
-            ->subject('Subject')
-            ->text('Some text')
-        ;
-
-        $envelope = new DelayedEnvelope($mail);
-        $envelope->setRecipients([new Address('[email protected]')]);
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
 
         $sendmailTransport = new SendmailTransport(self::FAKE_SENDMAIL);
         $sendmailTransport->send($mail, $envelope);
@@ -93,11 +86,90 @@ public function testRecipientsAreUsedWhenSet()
     }
 
     public function testThrowsTransportExceptionOnFailure()
+    {
+        $this->skipOnWindows();
+
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
+
+        $sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
+        $this->expectException(TransportException::class);
+        $this->expectExceptionMessage('Process failed with exit code 42: Sending failed');
+        $sendmailTransport->send($mail, $envelope);
+
+        $streamProperty = new \ReflectionProperty(SendmailTransport::class, 'stream');
+        $streamProperty->setAccessible(true);
+        $stream = $streamProperty->getValue($sendmailTransport);
+        $this->assertNull($stream->stream);
+    }
+
+    public function testStreamIsClearedOnFailure()
+    {
+        $this->skipOnWindows();
+
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
+
+        $sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
+        try {
+            $sendmailTransport->send($mail, $envelope);
+        } catch (TransportException $e) {
+        }
+
+        $streamProperty = new \ReflectionProperty(SendmailTransport::class, 'stream');
+        $streamProperty->setAccessible(true);
+        $stream = $streamProperty->getValue($sendmailTransport);
+        $innerStreamProperty = new \ReflectionProperty(ProcessStream::class, 'stream');
+        $innerStreamProperty->setAccessible(true);
+        $this->assertNull($innerStreamProperty->getValue($stream));
+    }
+
+    public function testDoesNotThrowWhenInteractive()
+    {
+        $this->skipOnWindows();
+
+        [$mail, $envelope] = $this->defaultMailAndEnvelope();
+
+        $sendmailTransport = new SendmailTransport(self::FAKE_INTERACTIVE_SENDMAIL);
+        $transportProperty = new \ReflectionProperty(SendmailTransport::class, 'transport');
+        $transportProperty->setAccessible(true);
+
+        // Replace the transport with an anonymous consumer that trigger the stream methods
+        $transportProperty->setValue($sendmailTransport, new class($transportProperty->getValue($sendmailTransport)->getStream()) implements TransportInterface {
+            private $stream;
+
+            public function __construct(ProcessStream $stream)
+            {
+                $this->stream = $stream;
+            }
+
+            public function send(RawMessage $message, ?Envelope $envelope = null): ?SentMessage
+            {
+                $this->stream->initialize();
+                $this->stream->write('SMTP');
+                $this->stream->terminate();
+
+                return new SentMessage($message, $envelope);
+            }
+
+            public function __toString(): string
+            {
+                return 'Interactive mode test';
+            }
+        });
+
+        $sendmailTransport->send($mail, $envelope);
+
+        $this->assertStringEqualsFile($this->argsPath, __DIR__.'/Fixtures/fake-failing-sendmail.php -bs');
+    }
+
+    private function skipOnWindows()
     {
         if ('\\' === \DIRECTORY_SEPARATOR) {
             $this->markTestSkipped('Windows does not support shebangs nor non-blocking standard streams');
         }
+    }
 
+    private function defaultMailAndEnvelope(): array
+    {
         $mail = new Email();
         $mail
             ->from('[email protected]')
@@ -109,9 +181,6 @@ public function testThrowsTransportExceptionOnFailure()
         $envelope = new DelayedEnvelope($mail);
         $envelope->setRecipients([new Address('[email protected]')]);
 
-        $sendmailTransport = new SendmailTransport(self::FAKE_FAILING_SENDMAIL);
-        $this->expectException(TransportException::class);
-        $this->expectExceptionMessage('Process failed with exit code 42: Sending failed');
-        $sendmailTransport->send($mail, $envelope);
+        return [$mail, $envelope];
     }
 }

src/Symfony/Component/Mailer/Transport/SendmailTransport.php

@@ -64,6 +64,7 @@ public function __construct(?string $command = null, ?EventDispatcherInterface $
         $this->stream = new ProcessStream();
         if (str_contains($this->command, ' -bs')) {
             $this->stream->setCommand($this->command);
+            $this->stream->setInteractive(true);
             $this->transport = new SmtpTransport($this->stream, $dispatcher, $logger);
         }
     }

src/Symfony/Component/Mailer/Transport/Smtp/Stream/ProcessStream.php

@@ -25,11 +25,18 @@ final class ProcessStream extends AbstractStream
 {
     private $command;
 
+    private $interactive = false;
+
     public function setCommand(string $command)
     {
         $this->command = $command;
     }
 
+    public function setInteractive(bool $interactive)
+    {
+        $this->interactive = $interactive;
+    }
+
     public function initialize(): void
     {
         $descriptorSpec = [
@@ -57,11 +64,15 @@ public function terminate(): void
             $err = stream_get_contents($this->err);
             fclose($this->err);
             if (0 !== $exitCode = proc_close($this->stream)) {
-                throw new TransportException('Process failed with exit code '.$exitCode.': '.$out.$err);
+                $errorMessage = 'Process failed with exit code '.$exitCode.': '.$out.$err;
             }
         }
 
         parent::terminate();
+
+        if (!$this->interactive && isset($errorMessage)) {
+            throw new TransportException($errorMessage);
+        }
     }
 
     protected function getReadConnectionDescription(): string