-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
When running as daemon worker, there are SSL errors when attempting to send email #4573
Comments
I would explore options besides it being a Laravel issue. It's a SwiftMailer exceptoin. |
Been noticing this on Forge, actually. Periodically the Queue will be unable to send an email and this error appears in my log and then it keeps on reappearing (as if the daemon never forgets it). And last night when a job failed, it was unable to make a MySQL connection. It is sporadic though and I have been unable to figure out why it sometimes fails to make these connections. |
Having this exact same problem, didn't start for me until I started running the queue as a daemon. Hoping this is updated with a potential solution. |
Daemon workers are long running PHP processes, which can have side effects from code that wasn't built with that in mind. It looks like SwiftMailer isn't built to be run long-term. To have a guess, it looks like it might not be catering for the fact the connection can drop and errors out. There's nothing Laravel can do about it, it's probably best to follow the discussion here or switch to one of the API drivers which don't have this problem (e.g. the built-in Mandrill support). |
@maknz thanks for the response, it was very enlightening. I've since moved away from the daemon approach, but thank you for shedding some light on what the actual issue was! |
While this is indeed a SwiftMailer issue, it might be noted in the Laravel Docs that SwiftMailer doesn't do well in a daemonized queue, or at least that we should be using API drivers when using a daemonized mail queue. The related SM thread is filled with comments from Laravel users. |
After some discussion with SwiftMailer developers, it looks like it should be solved from Laravel side - either by reinitializing Swift_Mailer object, either by closing Swift_Transport class object to close SMTP connection (or any other). |
We're open to pull requests. :) |
@GrahamCampbell can you please at least mention in docs, that SwiftMailer should not be used with queue deamon worker, but used with queue listener instead? |
If you feel this need to be addressed in the docs, feel free to send a Pull Request (as mentioned by @GrahamCampbell) to https://github.com/laravel/docs |
@YOzaz <3 |
So, as PR didn't go through - made a package: |
@YOzaz Why didn't the PR go through? |
@lukepolo I think because it may be undesired functionality. E.g. you may not want to do auto-restart for SMTP because of specific server configuration. But Laravel authors may answer better here ;) |
@YOzaz I actually tried your fix and still didn't work - gave up and moved back to regular queue, not daemon and it is such a CPU hog as it reloads framework every 5 seconds. Have you have any further feedback on your fix? Get same errors. |
@d4vidmiller which Laravel version are you using? |
Even in L5 there are still problems with this.
There is an A possible solution would be to put a try-catch around the $this->forceReconnection(); call in Example: /**
* Send a new message using a view.
*
* @param string|array $view
* @param array $data
* @param \Closure|string $callback
* @return void
*/
public function send($view, array $data, $callback)
{
try {
$this->forceReconnection();
} catch (\ErrorException $e) {
}
//... Any thoughts on this? It it worth a pull request? |
From my experience, ideal code should look something like this: $transport = $this->getSwiftMailerTransport();
if ( ! $transport->isStarted() )
{
$transport->start();
return;
}
try
{
// Send RESET to restart the SMTP status and check if it's ready for running
$transport->reset();
}
catch (Exception $e)
{
// In case of failure - let's try to stop it
try
{
$transport->stop();
}
catch (Exception $e)
{
// Just start it then...
}
$transport->start();
} ... because SwiftMailer may fail in various places. |
@schanzel that's my point -
Proper implementation would be to use |
Yah im still having these issues with 5.2 |
having the same issue here with laravel 5.1, any updates on that ? |
Any updates? |
Same issue here, v5.1.35 - tried scheduling an hourly restart of daemonized queue workers, but the problem persists. |
I had to restart every 15 min and seems to have been a good temp fix. |
Well, I tested all solutions include described in this thread Seems the only appropriate solution is stopping transport after each task (it's looks like the transport connects automatically on next task): \Mail::getSwiftMailer()->getTransport()->stop(); I tried to reset daemons after couple failed attempts, but after some time I got timeouts errors from mailgun so it didn't help me. This is all my code for the job handler (worked fine in prod for a week, php7). Catching errors/stopping daemon instance seems unnecessary, but I keep it for now (hope they can helps somebody) : public function sendOrderCompleted(RabbitMQJob $job, array $data)
{
try {
\Mail::send(
$data['template'],
$data['body'],
function ($message) use ($data) {
$message
->to($data['to'])
->subject($data['subject']);
}
);
} catch (\ErrorException $e) {
/**
* Workaround
*
*/
if ($job->attempts() >= self::ATTEMPTS_BEFORE_EXIT) {
$this->logger->info(
'Resetting daemon after ErrorException:'. $e->getMessage()
. 'Job: sendOrderCompleted. Job data: ' . json_encode($data)
);
// Attempts++
$job->release();
// Kill current daemon instance
exit(0);
}
$this->logger->info('Catches ErrorException in sendOrderCompleted: ' . $e->getMessage());
throw new \RuntimeException('ErrorException in sendOrderCompleted');
} catch (\Throwable $e) {
$this->logger->info(
'Throwable in sendOrderCompleted: '
. $e->getMessage()
. ' Data: '
. json_encode($data)
);
throw $e;
}
$job->delete();
// stopping mail transport
\Mail::getSwiftMailer()->getTransport()->stop();
} |
Maybe we just need the worker daemon to keep track of the number of swift exceptions, and kill itself for us? |
Is there a reason why the SMTP connection needs to be kept alive after the job has ran? Sounds like the equivalent of keeping a HTTP connection alive. E.g. |
I think closing the connection after sending an email would be the correct way to fix this. At least it sounds more sane than trying to close the connection before sending an email and calling that a forced reconnection. As @drcreazy's solution seems to work I've submitted a corresponding pull request for the 5.1 branch (see #13583). If approved I'll submit the the changes for 5.0 and 5.2 as well. |
…ssue #4573) (#13583) * Issue #4573 - close mailer connection after sending message - fix return statements and related phpdoc * Minor Fixed code style according to StyleCI * Revert return type fixes * Minor fixed code style according to StyleCI * Minor StyleCI still not happy... :-( * - Reverted removal of Mailer::forceReconnection() method - wrap try-finally around swift->send(); transport->stop(); * Minor: remove temporary result variable * Use private field instead of public getter for swift mailer
I upgraded from 4.1 to 4.2.
Running as queue:work would create SSL errors, but not when running queue:listen.
Using smtp driver with mailgun as the host.
The text was updated successfully, but these errors were encountered: