Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[8.x] Avoid SwiftMailer forceReconnection when not running from a queue daemon #36709

Closed
wants to merge 2 commits into from

Conversation

alim-shapiev
Copy link

I have noticed that emails that are sent in bulk all use separate connections (even though they are not simultaneous, there can be a limit of connections per minute per IP set for an SMTP server). As I've found in multiple pull requests (i.e., #4573), there has been a problem with long lasting SMTP connections due to some problems with SwiftMailer and a simple solution for it:

protected function sendSwiftMessage($message)
{
    $this->failedRecipients = [];

    try {
        return $this->swift->send($message, $this->failedRecipients);
    } finally {
        $this->forceReconnection();
    }
}

However, these problems are only encountered when a program is executed from a queue daemon (php artisan queue:work or php artisan queue:listen), so there can be a compromise between dealing with the SSL broken pipe error and sending multiple emails.

I propose setting a flag (or checking somehow else, if there's a better way for it) for when the script is executed from a queue daemon, then, when an email is sent via SMTP and the flag is not present, the SMTP connection will no longer be stopped. This would help in CRON jobs or in case of synchronous mail sending and wouldn't ruin queues a second time.

@alim-shapiev
Copy link
Author

As I can see, the tests are failing due to ReflectionException: Class config does not exist.
Is this because the config is not available inside the framework's core somehow?

@@ -520,7 +521,9 @@ protected function sendSwiftMessage($message)
try {
return $this->swift->send($message, $this->failedRecipients);
} finally {
$this->forceReconnection();
if (Config::get('app.is_run_from_queue_daemon', false)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use a facade like this here since the mailer component also can act as a standalone component.

@@ -116,6 +117,10 @@ public function __construct(QueueManager $manager,
*/
public function daemon($connectionName, $queue, WorkerOptions $options)
{
if ($connectionName !== 'sync') {
Config::set('app.is_run_from_queue_daemon', true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. We can't use a facade here.

@driesvints
Copy link
Member

Going to close this one as tests are indeed failing because of the facades. Feel free to re-attempt with a different solution and passing tests 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants