Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Failed() method not called on failing jobs. [5.0] [5.1] #9799

Closed
Shkeats opened this issue Jul 31, 2015 · 27 comments
Closed

Failed() method not called on failing jobs. [5.0] [5.1] #9799

Shkeats opened this issue Jul 31, 2015 · 27 comments

Comments

@Shkeats
Copy link

Shkeats commented Jul 31, 2015

I and a number of others have noticed that the failed method does not seem to get called on jobs that fail when being executed by workers off the queue. The failure is detected by laravel because the jobs are added to the failed_jobs table however the failed() method on the job is not called and the Queue::failing() event doesn't seem to be triggered.

Seems to be an issue in both 5.0 and 5.1.

Possibly related to #9473 which was closed by @GrahamCampbell

https://laracasts.com/discuss/channels/general-discussion/queuefailing-laravel-5

http://stackoverflow.com/questions/31305571/laravel-5-1-failed-queued-jobs-fails-on-failed-method-prevents-queue-failure

@GrahamCampbell
Copy link
Member

This is the expected behaviour. We only fire that event if firing the job failed.

@GrahamCampbell
Copy link
Member

If we crashed before we started firing the job, no event will be fired.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell I don't quite understand what you mean by firing the job failed. Is this if no handler is found? Could you give an example?

In my problem an uncaught exception is thrown inside the handler, the worker detects the exception, logs it and adds the job to the failed_jobs table. So far, as expected. However the failed event doesn't happen. Surely that job counts as failed and should trigger it's failed() method?

If there is some distinction between my scenario and some other sort of failure it didn't come accross from the docs.

@GrahamCampbell
Copy link
Member

Firing the job occurs when laravel actually calls the handle/fire method on your job. If it doesn't get that far, then no events are fired.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell OK but my job doesn't have a handle() method it has a handler that Laravel finds using the naming convention. The failure occurs within the handler for the job.

So in my scenario the Handler class is found, the handle() method on my handler is called successfully, there is an exception thrown somewhere inside the handle() method and the job fails.

Does the failed event only support self handling jobs?

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

To further clarify. I'm on 5.0 so I have app/Commands with MyCommand.php. It implements ShouldBeQueued and uses InteractsWithQueue, SerializesModels.

I new it up and dispatch it from a controller with $this->dispatch($myCommand).

The queue::listen daemon picks it up from the queue and finds the MyCommandHandler class in Handlers/Commands, calls the handle method passing the job into it. The job is then executed as expected. Unless there is an exception. In which case, see above.

@GrahamCampbell
Copy link
Member

The event should be fired, and it does for me. I can't replicate. Perhaps you have a typo?

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell OK thanks for responding. I'll investigate a bit more and see if I can replicate in a new project for you.

I would suspect user error too but the job/queue/handler system is functioning normally as long as there is no exception thrown so I can't think where I could introduce a typo without otherwise breaking it. I've checked the spelling of my failed() method.

I'm using Redis for a queue driver and queue::listen on daemon mode, but it does the same without the daemon flag.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell OK I've worked out what was happening. My failed() method was on the job class as per the docs, but in this use case the failed() method on the handler class is what gets called. By implementing the method there I managed to acheive the expected functionality.

http://laravel.com/docs/5.1/queues#dealing-with-failed-jobs

Assuming this is expected behaviour but could be stated more clearly?

@GrahamCampbell
Copy link
Member

I think the 5.0 docs probably explain it better. 5.1 pushes self handling more.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

I don't want to do the pull request for 5.1 as I'm not familiar with the new folder structure.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell Before I open a seperate issue, is there anyway to access the job instance from inside the failed() method on a handler? It seems like the handler class has been reinstantiated when failed() is called so I can't access the class variables that I set when handle() was called.

Since (correct me if I'm wrong) the job isn't passed back into the failed() method I can't actually do anything as I have no information about the job that failed.

Maybe conceptually the failed() method should be called on the job class even if handlers are being used?

@GrahamCampbell
Copy link
Member

If you want access, you could set it as a property on the handler from within your handle method.

@GrahamCampbell
Copy link
Member

I personally don't like the idea of a failed method though. I'd recommend implementing this using command middleware.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell Unfortunately the property route doesn't work. Like I said, it seems like the handler is getting reinstantiated at some point causing that variable to be lost.

 <?php
 class MyHandler extends CommandHandler
{
 protected $myJob;

 public function handle(MyJob $myJob)
 {
     //Set in handle method.
    $this->myJob = $myJob;
 }

 public function failed(){
     //Can't retrieve here.
     dd($this->myJob);
 }
}

@GrahamCampbell
Copy link
Member

You need to bind your handler as a singleton to the container.

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

In the end the most elegant solution I could find to get the the failed() method to trigger on the job class itself was to add to the below to the boot() method of EventServiceProvider.php. Catching the full fail event that is fired and then digging out the command/job and unserializing it to call the failed() method.

Queue::failing(function($connection, $job, $data)
        {
            $command = (unserialize($data['data']['command']));
            $command->failed();
        });

There goes my afternoon but I hope this helps someone!

Edit: This of course gives you access to all the data that was set in the intial job/command so you can carry out the necessary logic for your job failure.

@GrahamCampbell
Copy link
Member

What was wrong with using middleware?

@Shkeats
Copy link
Author

Shkeats commented Jul 31, 2015

@GrahamCampbell I'm sure nothing! I'm just not familiar with middleware in Laravel so the above was the shortest way around the problem for now. If I get a middleware solution put together I'll post an example back here too. Thanks for the help.

@nosun
Copy link

nosun commented Nov 2, 2015

mark here, the same issue

@etiennemarais
Copy link

Seems like this has changed a little in 5.2.

Queue::failing(function (JobFailed $event) {
            // $event->connection
            // $event->$job
            // $event->$data
        });

@vafrcor
Copy link

vafrcor commented Mar 21, 2017

Still have this issue on Laravel 5.2.x (latest)
Condition:

  • Queue Worker Daemon triggered by Supervisord
  • Each Job has been triggered with long time runtime (heavy task using loop)
  • Failed Job recorded on failed_jobs table, but no entry on Laravel Log file.

@stdclass
Copy link

Still apparrent in Lumen 5.4.7

@hadifarnoud
Copy link

hadifarnoud commented Oct 16, 2019

In the end the most elegant solution I could find to get the the failed() method to trigger on the job class itself was to add to the below to the boot() method of EventServiceProvider.php. Catching the full fail event that is fired and then digging out the command/job and unserializing it to call the failed() method.

Queue::failing(function($connection, $job, $data)
        {
            $command = (unserialize($data['data']['command']));
            $command->failed();
        });

There goes my afternoon but I hope this helps someone!

Edit: This of course gives you access to all the data that was set in the intial job/command so you can carry out the necessary logic for your job failure.

does this give me job data in logs? I want to have the data of failed jobs with Laravel 5.1 so I put this in EventServiceProvider

public function boot(DispatcherContract $events)
    {
        parent::boot($events);
        Queue::failing(function($connection, $job, $data)
        {
            $command = (unserialize($data['data']['command']));
            $command->failed();
        });
        //
    }

@hadifarnoud
Copy link

it should be like this.

use Illuminate\Support\Facades\Queue;

    public function boot(DispatcherContract $events)
    {
        parent::boot($events);
        Queue::failing(function($connection, $job, $data)
        {
            $command = (unserialize($data['data']['command']));
            $command->failed();
        });
        //
    }
}

@xwiz
Copy link

xwiz commented Apr 16, 2021

In my case I forgot to import Exception type hinted in failed method.

@chadgrigsby
Copy link

In my case, I need to restart the queue worker.. ugh!

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

No branches or pull requests

9 participants