laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.34k stars 10.97k forks source link

[L4.2] Mail keeps sending older message, queue job not receiving correct payload? #9881

Closed georgeboot closed 8 years ago

georgeboot commented 9 years ago

I'm using Laravel 4.2 with the Mandrill driver, and beanstalkd as queue driver, all on a Forge server.

In my app I queue emails with the following syntax:

Mail::queue('emails.welcome', $data, function($message) use ($data)
{
    $message->to($data['email']);
    $message->subject('Registration confirmation for ' . $data['event_short_title']);

});

Now, from times to times the emails ending up actually sent have the correct to and subject, but the rendered template is incorrect. It actually is a email that was sent weeks ago. So far I was able to stop the issue by restarting the queue worker, but every time it returns after a while.

I searched though the code myself, but was not able to find any issue. I captured some payload but that looks 100% fine. In fact, I captured some payload, let it execute by the worker and found that the addressee and object of the sent email were correct, the body was not.

Below is an example of a captured payload. The email that was sent by the hander contained a different content then specified in the below payload.

{
    "job":"mailer@handleQueuedMessage",
    "data":{
        "view":"emails.registrationConfirmation",
        "data":{
            "firstname":"George",
            "event_short_title":"Slanghoek MTB Triathlon",
            "event_id":1641,
            "registration_id":1034871,
            "mail_reason":"You are receiving this email because you registered for Slanghoek MTB Triathlon",
            "organiser_id":5,
            "fullname":"George Boot",
            "email":"george@xxxxxxxx.nl"
        },
        "callback":"C:38:\\"Illuminate\\\\Support\\\\SerializableClosure\\":547:{
            a:2:{
                i:0;s:160:\\"function ($message) use($data) {
                    \\n    $message->to($data[
                        \'email\'
                    ],
                     $data[
                        \'fullname\'
                    ])->subject(\'Registration confirmation for \' . $data[
                        \'event_short_title\'
                    ]);\\n
                };\\";i:1;a:1:{
                    s:4:\\"data\\";a:8:{
                        s:9:\\"firstname\\";s:6:\\"George\\";s:17:\\"event_short_title\\";s:23:\\"Slanghoek MTB Triathlon\\";s:8:\\"event_id\\";i:1641;s:15:\\"registration_id\\";i:1034871;s:11:\\"mail_reason\\";s:79:\\"You are receiving this email because you registered for Slanghoek MTB Triathlon\\";s:12:\\"organiser_id\\";i:5;s:8:\\"fullname\\";s:11:\\"George Boot\\";s:5:\\"email\\";s:17:\\"george@xxxxxxxx.nl\\";
                    }
                }
            }
        }"
    }
}

Any thoughts what might be going on here? I almost can't image this is a Laravel bug, but I'm also pretty confident I'm not doing something wrong.

GrahamCampbell commented 9 years ago

Laravel 4.2 is not supported, sorry.

AchoArnold commented 9 years ago

Wow so 4.2 is not supported so soon? Open source rocks! Get on or get lost :wink:

georgeboot commented 9 years ago

@GrahamCampbell I understand there is no official support for L4.2 anymore, but some help would be appreciated. We're on our way moving over to L5.1, but it takes some time.

GrahamCampbell commented 9 years ago

Wow so 4.2 is not supported so soon?

4.2 hasn't been supported at all for the last 9 months...

GrahamCampbell commented 9 years ago

For all non-LTS releases, support terminates immediately following a new minor/major release.

gwlortscher commented 9 years ago

Seeing this exact same issue on Laravel 5.1 with daemon queue using Beanstalkd, Supervisord and Mandrill.

georgeboot commented 9 years ago

@gwlortscher @GrahamCampbell Seems this issue should be re-opened

GrahamCampbell commented 9 years ago

I can't replicate.

gwlortscher commented 9 years ago

It looks like perhaps it's triggered when an error occurs while rendering a view in the queue job.

In this case it was a "route not defined" error, after which all views rendered for any queued email are that of the most recent successful email before the error.

georgeboot commented 9 years ago

Thanks @gwlortscher for the research!

In my case I'm not sure though if it is caused by a not defined route. In my experience it started after a while, it was able to send correct emails. When I did a worker restarts, email work fine for x minutes, and then they revert to that one special mail being send over and over.

vladrusu commented 9 years ago

I confirm this bug. It drives me nuts. "Thanks" to it, today I sent a newsletter to 7000+ members, with correct to/subject, but with the same body i.e. "Dear John Smith". You don't know to imagine the reaction of my clients.

I use beanstalkd in connection with MailGun.

Given @georgeboot uses Mandrill, I suspect the bug lies somewhere within Beanstalkd implementation. I previously used IronMQ with no issues.

Apparently, when sending from a Queue, Laravel sometimes attaches in Mail::send() the same body, over and over again (I repeat, the Subject and To are sent correctly, but with the same body...).

I saw the issue occurs mostly when you change a file or two in your code.... the beanstalkd queue system goes then nuts.

I usually restart the queue every often I make changes to code on my server.

But this is not a solution, but an flimsy workaround.

@taylorotwell, please help us to fix this!

crynobone commented 9 years ago

I usually restart the queue every often I make changes to code on my server.

Are you using php artisan queue:work --daemon or php artisan queue:listen?

Apparently, when sending from a Queue, Laravel sometimes attaches in Mail::send() the same body, over and over again (I repeat, the Subject and To are sent correctly, but with the same body...).

Which other packages did you install in your application, or rather does any of those packages related to view caching?

vladrusu commented 9 years ago

I use the queue as a daemon:

[running Ubuntu Server] Put this in /etc/init/myqueue.conf:

description "work queue" start on filesystem stop on runlevel [!2345] respawn respawn limit 5 2 script exec /usr/bin/php /var/www/[pathtomysite]/artisan queue:work --daemon --queue=high,medium,low end script

I run the queue daemon with: start myqueue

.. and stop it with: stop myqueue

After each change on my site, usually I run a pair of: stop myqueue start myqueue

... just to be sure I don't run into this bug. Of course, a fix would be much more than welcome.

crynobone commented 9 years ago

If you using php artisan queue:work --daemon you need to run php artisan queue:restart whenever you deploy new code/change configuration, because the daemon worker is executed before the deployment, hence using the old code (until it timeout or fail, then your code will respawn the daemon using new deployed code).

That shouldn't be related to this issue.

vladrusu commented 9 years ago

Ok, I understand using old code... But why sending the same body in Mail::send()? This is like a critical, sensitive bug to me... It it used the old code, before the update, it wouldn't be a problem...

georgeboot commented 9 years ago

@crynobone I also did a temporary solution of restarting my queues every 5 minutes, but that is a temporary solution.

I agree with @vladrusu that the bug probably lays somewhere in the beanstalkd implementation.

crynobone commented 9 years ago

@georgeboot @vladrusu

I'm also using beanstalkd, queue:work --daemon and Amazon SES, and we roughly sent 400-500 notification emails to our users on the daily basis (overspeeding alert) which include unique user information and vehicle speeding detail. If there is an issue, it should have affected our operation.

p/s: We're on Laravel 5.1

robclancy commented 8 years ago

Got the same issue.

robclancy commented 8 years ago

Also @GrahamCampbell you come off as a complete dick when people have a clear bug and all you have to say is "I can't reproduce". Which is clear, because if you could it would probably be fixed.

Stop assuming nothing is a bug or relevant to current releases. Unless your job is to be the "warden of closing issues that aren't the version I want them to be even though the issue could be in latest version".

georgeboot commented 8 years ago

@robclancy I think we all experience some frustration here, but lets solve this as gentlemans!

robclancy commented 8 years ago

@georgeboot I think being a "gentlemen" goes out the door when a security leak is ignored and because of it I get woken up early in the morning to have to address it.

robclancy commented 8 years ago

Oh and since this is a security issue it should be fixed in 4.2 regardless of support policy. The issue referenced 18 days ago has an apparent workaround and reproduction steps.

GrahamCampbell commented 8 years ago

Please contact Taylor directly regarding security issues. Otherwise, if there is a "bug" in 5.1 or 5.2, please create a new, clear issue.

jaketoolson commented 8 years ago

This has been happening to us for months on end (4.2 because we can't yet upgrade to 5), and I'm thrilled to have found this thread and the most recent one in 5.1 #11892

I'm curious to identify a solution and see the fix because I had to turn down the "errors and retries" and move on. We use Iron.io for our queue

taylorotwell commented 8 years ago

I just pushed up a fix on 5.1 and 5.2... working on one for 5.0 / 4.2 now.

taylorotwell commented 8 years ago

Alright I have fixed out on 4.2 through 5.2. Basically if we hit an exception during the View->render() routine we just flush the sections and then re-throw the exception. Is anyone that is experiencing the issue able to pull in my fix using "dev" stability in Composer and confirm the fix?

taylorotwell commented 8 years ago

Fixed.