[5.1] Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573) #13583

schanzel · 2016-05-17T13:33:10Z

Closing swift mailer connection after sending the mail (see #4573 (comment))

- close mailer connection after sending message - fix return statements and related phpdoc

Fixed code style according to StyleCI

fixed code style according to StyleCI

StyleCI still not happy... :-(

GrahamCampbell · 2016-05-17T13:50:26Z

src/Illuminate/Mail/Mailer.php

@@ -392,7 +378,10 @@ protected function sendSwiftMessage($message)
        }

        if (! $this->pretending) {
-            return $this->swift->send($message, $this->failedRecipients);
+            $result = $this->getSwiftMailer()->send($message, $this->failedRecipients);
+            $this->getSwiftMailer()->getTransport()->stop();


Please do a try finally here.

GrahamCampbell · 2016-05-17T13:51:24Z

This will kill performance for someone sending lots of messages.

GrahamCampbell · 2016-05-17T13:51:57Z

src/Illuminate/Mail/Mailer.php

@@ -392,7 +378,10 @@ protected function sendSwiftMessage($message)
        }

        if (! $this->pretending) {
-            return $this->swift->send($message, $this->failedRecipients);
+            $result = $this->getSwiftMailer()->send($message, $this->failedRecipients);


please don't modify this line

schanzel · 2016-05-17T14:00:32Z

The connection is stopped on each mail being sent, regardless of this change. The only difference here is the stop() being called after sending an email instead of calling it beforehand. I don't see any performance regressions here.

- wrap try-finally around swift->send(); transport->stop();

lifeofguenter · 2016-05-17T14:10:22Z

Ok, so what is better: unreliable application (restarting the queue-workers via cronjob does not make an application more robust) - or faster delivery - which can be achieved running multiple queue-workers in parallel?

The correct way would be to detect a faulty connection and then only reconnect then (which seems to be the current intend, but obviously not working).

Will take a stable application that I can scale predictably any-time over some faulty hacked functionality. But that's just me :)

GrahamCampbell · 2016-05-17T15:15:22Z

src/Illuminate/Mail/Mailer.php

+            try {
+                return $this->swift->send($message, $this->failedRecipients);
+            } finally {
+                $this->getSwiftMailer()->getTransport()->stop();


$this->swift->getTransport()->stop()

Kind of inconsistent here anyway since Mailer::forceReconnection() uses the public getter, but I've changed it according to your comment.

taylorotwell · 2016-05-18T13:40:54Z

Have y'all tested this with your own applications to verify it fixes the problem?

YOzaz · 2016-05-18T13:55:23Z

@taylorotwell @lifeofguenter how about implementing my try-catch multiblocks?
https://github.com/YOzaz/Laravel-SwiftMailer/blob/master/src/YOzaz/LaravelSwiftmailer/Mailer.php#L155
It basically reconnects when required. Note, this was written by SwiftMailer author, so should cover all possible issues. Therefore it won't autoclose before or after sending, and shouldn't have an impact for performance.
As a side note, this won't solve SSL connection problems like this one: YOzaz/Laravel-SwiftMailer#1 (comment)

By the way, PRs are still here, may need some adjustment though:
#8222
#8225

taylorotwell · 2016-05-18T15:03:57Z

src/Illuminate/Mail/Mailer.php

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


Why not just call "forceReconnection" here?

@taylorotwell the problem is that stop() may throw an exception, but even if it'll be catched - it won't reset transport. You need to call either reset(), either stop() > start().

Because calling "forceReconnection" would not make sense form a semantic point of view.

YOzaz · 2016-05-18T15:25:01Z

Guys, you're missing the point here. STOPping SMTP connection is slow and expensive operation, and shouldn't be done every time for every email to be sent. RESET is much faster and suits here better - however, in case of timeout it won't succeed.

taylorotwell · 2016-05-18T15:26:05Z

@YOzaz if you have a better solution then PR it.

lifeofguenter · 2016-05-18T15:37:16Z

@YOzaz as said, what does it help if the fix is only suited for certain situations? Would still make email sending via SMTP super unreliable.

If you need to send higher volumes, you still have the possibility to launch multiple queue-workers or outsource via services such as mandrill, that offer an async http API for sending bulk mails.

taylorotwell · 2016-05-18T15:39:35Z

@schanzel are you actually running this fix in production to verify that it works?

schanzel · 2016-05-18T15:42:50Z

@YOzaz What you are suggesting sounds a lot like premature optimisation and would add lots of unnecessary complexity. What you would normally want to do is open a connection, send a mail and finally close the connection. Handling any performance or network related problems should not be in focus here. Anything related to that should be handled by a dedicated connection pool, if needed (IMO).

taylorotwell · 2016-05-18T15:43:46Z

Can someone just answer the question as to whether this fix has actually been tested to solve the problem? 😆

YOzaz · 2016-05-18T15:51:25Z

@taylorotwell PR is here: #13609
And yes, I have tested it in my package, and code is copy-paste from there. Haven't tested with Laravel Framework 5.1 yet, tough.

YOzaz · 2016-05-18T15:55:51Z

@schanzel you're basically right, but - following this ideology SMTP connection in daemon workers should be handled same way like DB reconnection, as suggested here: https://laravel.com/docs/5.1/queues (see Coding Considerations For Daemon Queue Listeners).
However, at the moment, Laravel developers have no way to force SMTP reconnection manually, unless forceReconnection() will become public and renamed to something like Mailer::reset().

taylorotwell · 2016-05-18T15:56:59Z

I should note that calling DB::reconnect is not needed in 5.2+…

On May 18, 2016, at 10:55 AM, Marijus Plančiūnas notifications@github.com wrote:

@schanzel https://github.com/schanzel you're basically right, but - following this ideology SMTP connection in daemon workers should be handled same way like DB reconnection, as suggested here: https://laravel.com/docs/5.1/queues https://laravel.com/docs/5.1/queues (see Coding Considerations For Daemon Queue Listeners).
However, at the moment, Laravel developers have no way to force SMTP reconnection manually, unless forceReconnection() will become public and renamed to something like Mailer::reset().

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #13583 (comment)

YOzaz · 2016-05-18T15:59:48Z

@lifeofguenter sending emails through multiple queue tubes does not solve the problem in general - which is handling SMTP connections for long-running processes.
My suggestion would to use reset() method on a transport, which is fast, and actually built for this. It may fail only on specific scenarios with TTL/SSL - therefore additional try-catch blocks are required.
Mandrill is a separate discussion, especially when it became paid. Trust me, it has it's own problems and possibilities as well 😄

taylorotwell · 2016-05-18T16:08:16Z

@schanzel still need to know if this has been tested on a real app.

YOzaz · 2016-05-18T16:19:28Z

Right, I'll leave PR as it is for now - Travis is failing because of change, therefore it needs more testing.

schanzel · 2016-05-19T09:26:17Z

Just deployed the fix at a larger scale to get somewhat more relibale information about whether or not this fix does the trick. The failures occure sporadically and are hard to reproduce. I'll get back to it once I got more information about it.

taylorotwell · 2016-05-27T14:46:31Z

Whats the status of this? We have this PR and @YOzaz's hanging out here and I'd like to get a solution merged in.

schanzel · 2016-05-30T09:53:24Z

We've had some internal problems which required a rollback, due to which this change was rollbacked also. I just deployed it again. I think I can give an answer by Wednesday or so. Please note that the rollback of the application was not related to this change ;-)

taylorotwell · 2016-06-06T14:26:20Z

Any updates on this?

dbpolito · 2016-06-07T14:37:37Z

This looks good to me, seems to be the right solution and fix the issue.

The only remaining change is remove method forceReconnection as it's not used anymore...

schanzel · 2016-06-07T14:38:24Z

Since this fix is on production for a few days now, there are no SSL errors so far. I'd take this as an indication that the issue would be solved by this PR.

schanzel · 2016-06-07T14:40:49Z

@dbpolito Removing forceReconnection was considered a breaking change, therefore I reverted the removal.

dbpolito · 2016-06-07T14:47:02Z

Gotcha.

I'm going to need to push this fix to my prod manually too... Looking forward to get this merged and released into 5.2! 👍

dbpolito · 2016-06-07T15:48:20Z

This is my temp fix:

Queue::after(function (JobProcessed $event) {
    Mail::getSwiftMailer()->getTransport()->stop();
});

Which may also be another approach for fixing this.

GrahamCampbell · 2016-06-07T15:49:03Z

This is my temp fix:

That can lead to terrible performance though.

taylorotwell · 2016-06-07T17:46:38Z

Define terrible performance? @GrahamCampbell

GrahamCampbell · 2016-06-07T17:54:59Z

Define terrible performance? @GrahamCampbell

If you turn off keeping the connection open, you have to negotiate everything. If negotiation takes 0.1 seconds, and sending an email takes 0.05, then sending 10 emails takes 3 times as long after this change.

overint · 2016-07-04T03:32:10Z

Can confirm it's now fixed, but emails do take a little longer to send.
Thanks everyone.

driade · 2016-12-09T10:00:24Z

Thanks to @dbpolito , stopped the connection at the end of every Job that sends an email and everything seems fine now. Using Laravel 4.2.

yincongcyincong · 2017-09-11T07:08:59Z

it solve my problem! thanks

schanzel added 3 commits May 17, 2016 14:35

Issue #4573

8500308

- close mailer connection after sending message - fix return statements and related phpdoc

Minor

2d3c63d

Fixed code style according to StyleCI

Revert return type fixes

777507d

schanzel mentioned this pull request May 17, 2016

When running as daemon worker, there are SSL errors when attempting to send email #4573

Closed

schanzel added 2 commits May 17, 2016 15:34

Minor

cadbc03

fixed code style according to StyleCI

Minor

e54da9a

StyleCI still not happy... :-(

GrahamCampbell reviewed May 17, 2016
View reviewed changes

GrahamCampbell changed the title ~~Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573)~~ [5.1] Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573) May 17, 2016

GrahamCampbell reviewed May 17, 2016
View reviewed changes

- Reverted removal of Mailer::forceReconnection() method

d6c259e

- wrap try-finally around swift->send(); transport->stop();

Minor: remove temporary result variable

201749c

GrahamCampbell reviewed May 17, 2016
View reviewed changes

Use private field instead of public getter for swift mailer

7a54388

YOzaz mentioned this pull request May 18, 2016

Broken pipe in StreamBuffer.php on line 236 swiftmailer/swiftmailer#490

Closed

taylorotwell reviewed May 18, 2016
View reviewed changes

taylorotwell closed this Jun 7, 2016

taylorotwell reopened this Jun 7, 2016

taylorotwell merged commit bb59455 into laravel:5.1 Jun 11, 2016

This was referenced Jun 12, 2016

[5.0] Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573) #13956

Closed

[5.2] Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573) #13957

Closed

Jubeki mentioned this pull request Aug 30, 2021

[9.x] Implement Symfony Mailer #38481

Merged

7 tasks

[5.1] Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573) #13583

[5.1] Yet another attempt to fix Swift Mailer related SSL failures (Issue #4573) #13583

Conversation

schanzel commented May 17, 2016

GrahamCampbell May 17, 2016 • edited Loading

Choose a reason for hiding this comment

GrahamCampbell commented May 17, 2016

GrahamCampbell May 17, 2016

Choose a reason for hiding this comment

schanzel commented May 17, 2016

lifeofguenter commented May 17, 2016

GrahamCampbell May 17, 2016

Choose a reason for hiding this comment

schanzel May 17, 2016

Choose a reason for hiding this comment

taylorotwell commented May 18, 2016

YOzaz commented May 18, 2016 • edited Loading

taylorotwell May 18, 2016

Choose a reason for hiding this comment

YOzaz May 18, 2016

Choose a reason for hiding this comment

schanzel May 18, 2016

Choose a reason for hiding this comment

YOzaz commented May 18, 2016

taylorotwell commented May 18, 2016

lifeofguenter commented May 18, 2016

taylorotwell commented May 18, 2016

schanzel commented May 18, 2016

taylorotwell commented May 18, 2016

YOzaz commented May 18, 2016

YOzaz commented May 18, 2016

taylorotwell commented May 18, 2016

YOzaz commented May 18, 2016 • edited Loading

taylorotwell commented May 18, 2016

YOzaz commented May 18, 2016

schanzel commented May 19, 2016

taylorotwell commented May 27, 2016

schanzel commented May 30, 2016

taylorotwell commented Jun 6, 2016

dbpolito commented Jun 7, 2016

schanzel commented Jun 7, 2016

schanzel commented Jun 7, 2016

dbpolito commented Jun 7, 2016

dbpolito commented Jun 7, 2016

GrahamCampbell commented Jun 7, 2016

taylorotwell commented Jun 7, 2016

GrahamCampbell commented Jun 7, 2016

overint commented Jul 4, 2016

driade commented Dec 9, 2016 • edited Loading

yincongcyincong commented Sep 11, 2017

GrahamCampbell May 17, 2016 •

edited

Loading

YOzaz commented May 18, 2016 •

edited

Loading

YOzaz commented May 18, 2016 •

edited

Loading

driade commented Dec 9, 2016 •

edited

Loading