dusterio / laravel-plain-sqs

Custom SQS connector for Laravel (or Lumen) that supports third-party, plain JSON messages
MIT License
131 stars 93 forks source link

Payload issues fix #5

Closed sebastianvilla closed 8 years ago

sebastianvilla commented 8 years ago

Suggested fix to open issue #4

We were tackling the issue from the wrong angle first, as we were trying to match Laravel's failed job payload format (uses CallQueuedHandler class), but the real issue was payloads were coming from the queue in one format, and would be put back in a different format (modified to [job, data] array) if they were retried after failing for example.

The solution was to only modify payloads that don't follow the [job, data] format and flag them with was_plain => true. By overriding the pushRaw method from the parent class, we can check messages/payloads before pushing them (back) to the queue, and if they include the was_plain flag we know we need to bring them back to their original format.

I am open to suggestions, but found this to be a good solution for my team and our use case in particular!

Thanks @dusterio!

dusterio commented 8 years ago

I checked this and so far it looks rational! I will merge and tag this tomorrow!

dusterio commented 8 years ago

I merged this and I think I managed to simplify it a bit - it works without an extra field ('was_plain')

sebastianvilla commented 8 years ago

I thought of that (you just modified the pushRaw override), but that will conflict when you dispatch new jobs as it will always strip out the "job" class in the payload, no?

dusterio commented 8 years ago

I've just tried it in my test app and it worked fine - I could dispatch, I could retry, etc. So far I couldn't find any bugs but if you do – please let me know!

sebastianvilla commented 8 years ago

There seems to be a bigger issue; when dispatching jobs onto the queue like so:

$this->dispatch((new DispatcherJob(["song" => "Test Song", "artist" => "unknown"])));

SQS receives the following payload:

{"job":"Illuminate\\Queue\\CallQueuedHandler@call","data":{"command":"O:36:\"Dusterio\\PlainSqs\\Jobs\\DispatcherJob\":6:{s:7:\"\u0000*\u0000data\";a:2:{s:4:\"song\";s:9:\"Test Song\";s:6:\"artist\";s:7:\"unknown\";}s:8:\"\u0000*\u0000plain\";b:0;s:6:\"\u0000*\u0000job\";N;s:10:\"connection\";N;s:5:\"queue\";s:23:\"songs-queue\";s:5:\"delay\";N;}"}}

When laravel-plain-sqs receives this message from the queue and it fails, it stores the payload in the failed_jobs table as:

{"job":"App\\Jobs\\ TestSQSJobHandlerPlain@handle","data":{"job":"Illuminate\\Queue\\CallQueuedHandler@call","data":{"command":"O:36:\"Dusterio\\PlainSqs\\Jobs\\DispatcherJob\":6:{s:7:\"\u0000*\u0000data\";a:2:{s:4:\"song\";s:9:\"Test Song\";s:6:\"artist\";s:7:\"unknown\";}s:8:\"\u0000*\u0000plain\";b:0;s:6:\"\u0000*\u0000job\";N;s:10:\"connection\";N;s:5:\"queue\";s:23:\"songs-queue\";s:5:\"delay\";N;}"}}}

Now, when you retry the job via php artisan queue:retry job_id TestSQSJobHandlerPlain@handle receives the payload in the wrong format. It expects a JSON object with song and artist, but instead it receives an array with job and data properties.

Have you experienced this behaviour on your end?

sebastianvilla commented 8 years ago

Apologies, this is not an issue. I was pushing jobs using the default queue driver onto this queue (I have 2 queue connections setup), so the payload was being formatted through the Laravel sqs driver, not laravel-plain-sqs.