PendingDispatch impede exception handling when dispatching jobs

[g4b0]

• Laravel Version: 9.39.0
• PHP Version: 8.1.11
• Database Driver & Version: Mysql 5.7

Description:

This bug originates in PendingDispatch destructor [1], where jobs are actually dispatched to the queues. Since the destructor method will be called as soon as there are no other references to a particular object, or in any order during the shutdown sequence, it is not guaranteed that the exception could be catched.

The problem is that when Laravel try to dispatch a job to a queue, the connection may fail (broker down, database not responding, Redis connection broken, ...) and the job should be treated as a failed job. For example in my current project we are using AMQP protocol, and in case of RabbitMQ not responding we would like to log the failure and store the job payload into the failed_job db table, without propagating the exception to the client.

[1] https://github.com/laravel/framework/blob/917abe82938ec332ceac2a2e662ccd983b6632fd/src/Illuminate/Foundation/Bus/PendingDispatch.php#L186-L195

Steps To Reproduce:

To implement this solution we wrote an abstract class from which all our jobs inherit:

abstract class BaseJob implements ShouldQueue
{
    use Dispatchable;
    use InteractsWithQueue;
    use Queueable;
    use SerializesModels;

    public static function dispatch(...$arguments): ?PendingDispatch
    {
        $self = new static(...$arguments);

        try {
            return new PendingDispatch($self);
        } catch (\Throwable $t) {
            // log failure
            // persist failed job to DB
        }

        return null;
    }
}

Unfortunately the catch will never be engaged, since the return inside the try implies that an object is instantiated, so the destructor call is delayed.

Possible workaround:

One possible solution is move the try/catch in the dispatch usage, something like that:

try {
    TestJob::dispatch();
} catch (\Throwable $t) {
    // log failure
    // persist failed job to DB
}

This solution is not optimal because:

• delegate problem management to the framework user, who may not be aware of the problem
• forces to duplicate the management code, we could use the Traits, but it is still not an acceptable solution
• moreover, if for some reason the dispatch return value were assigned to a variable, the exception would not be manageable even at this point

Desired solution

I think that the best way to solve this problem is to let the user the ability to bypass the destructor trick, maybe exhuming the deprecated dispatchNow method, giving it the following meaning: push the job to the queue NOW, not later when you will be destroyed, then work on it in ASYNC mode.

[github-actions[bot]]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub. If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[g4b0]

Hello @driesvints, We tried digging deeper in this bug, but the problem is that the PendingDispatch is widely used in the framework, and a possible PR can be very big and intrusive. I think that this issue should be addressed by an expert Laravel framework developer, maybe @taylorotwell in person, because queue engine is a very critical and complicate piece of code.

In userland we partially patched the problem extending Illuminate\Foundation\Bus\Dispatchable dispatchNow() deprecated method in a BaseJob abstract class from which all our jobs inherit [1]

Our solution works, but it doesn't solve all the problems. For example in our project we are using Illuminate\Notifications\Notifiable trait, that under the hood dispatches jobs using \Illuminate\Contracts\Bus\Dispatcher dispatch() method, de facto bypassing our workaround, and the only solution is to duplicate our hack try/catching each notify() call.

Another problem with our workaround is that we loose the PendingDispatch method chaining, since we can't instantiate it.

A possible solution should be providing in some manner (via config?) a callable to the PendingDispatch objects, and use it in the distructor when catching an exception.

Furthermore it should be useful to provide a configuration allowing to call the dispatch() and dispatchAfterResponse() immediately and not during the object destruction, because it may be too late and for example the DB connection can already be closed, impeding to store the job in failed_jobs table.

I think that using a broker different from DB, but having the queue driver that fallback in DB for failed jobs can be a great enhancement, eliminating a possible single point of failure and resulting in a more robust pub/sub implementation, essential for using Laravel in a modern microservice architecture.

[1]

abstract class BaseJob implements ShouldQueue
{
    use Dispatchable;
    use InteractsWithQueue;
    use Queueable;
    use SerializesModels;

    /**
     * Dispatch the job with the given arguments in a sync way.
     * In case of network failure or queue broker not ready it save the
     * job as failed.
     *
     * <b>WARNING</b>
     *
     * Parent method returns a PendingDispatch, but luckily it does not declare a return value.
     * Read comments in method implementations to know why we did this ugly hack.
     * As side effect it's not compatible with original dispatch method chaining.
     */
    public static function dispatchNow(...$arguments): void
    {
        $self = new static(...$arguments);

        try {
            // PendingDispatch will push the job on queue during
            // __destruct(). From php docs: The destructor method will
            // be called as soon as there are no other references to a
            // particular object, or in any order during the shutdown
            // sequence.
            // Using new without assigning the object to any variable will
            // trigger the __destruct() immediately, so we can catch the
            // eventual exception raised in case of networking problems.
            new PendingDispatch($self);
        } catch (\Exception $e) {
            $payload = JobHelper::createPayloadArray($self, $self->serializeJob($self));

            $data = [
                'uuid' => $payload['uuid'],
                'connection' => $self->connection ?? config('queue.default'),
                'queue' => $self->queue,
                'payload' => json_encode($payload, \JSON_UNESCAPED_UNICODE),
                'exception' => (string) mb_convert_encoding($e->getMessage(), 'UTF-8'),
                'failed_at' => Date::now(),
            ];

            Log::system()->error('Error dispatching job', $data);

            // Persist the dispatch intent as a failed job in case of exception.
            // @see \Illuminate\Queue\Failed\DatabaseFailedJobProvider::log()
            if ($self->connection !== 'database') {
                DB::table('failed_jobs')->insert($data);
            }
        }

        // Missing return value is correct because returning the PendingDispatch
        // will bypass the above exception handling.
    }

}

[timacdonald]

@g4b0 can you instead use Bus::dispatch(new Job()); and just not touch then pending dispatch side of things?

[g4b0]

Hi @timacdonald , thanks for the hint. For now we are happy with our dispatchNow() implementation, that basically does the same of Bus::dispatch(new Job()).

But the problem with PendingDispatch IMHO has to be addressed, since loosing exception at this level can lead to production problems if the developers are not aware of it.

[rodrigopedra]

What about delegating any exceptions to the job failed method?

https://laravel.com/docs/9.x/queues#cleaning-up-after-failed-jobs

This won't still automatically add the job to the failed_jobs table. Which IMO it shouldn't as the job never got in the queue in the first place as it failed while dispatching.

But the developer could try redispatching it with a delay.

Perhaps it could call a new method for this case to not make failed perform a double duty. failedDispatching, maybe?

// Illuminate\Foundation\Bus\PendingDispatch

    public function __destruct()
    {
        if (! $this->shouldDispatch()) {
            return;
        }

        try {
            if ($this->afterResponse) {
                app(Dispatcher::class)->dispatchAfterResponse($this->job);
            } else {
                app(Dispatcher::class)->dispatch($this->job);
            }
        } catch (\Throwable $exception) {
            if (method_exists($this->job, 'failed')) {
                $this->job->failed($exception);
            } else {
                throw $exception;
            }
        }
    }

[g4b0]

Hi @rodrigopedra, thanks for your contribution to the problem.

I agree with you for a new method to call when dispatch problems occurs, since mixing failed dispatch with failed execution logic could be confusing. failedDispatching could be a good name from my point of view.

On the other side I disagree with you about not to put the job in failed_jobs table when error occurs during dispatch. Ok retrying it, but what happens if it fails again? Remember that usually job dispatch occurs in real time, with the client waiting for a response.

Finally the problem of your proposed code is the same of the actual code: the way the desctructor works. From the online docs: "The destructor method will be called as soon as there are no other references to a particular object, or in any order during the shutdown sequence". If our failedDispatching method is invoked during the shutdown sequence it is very likely that he will fail too, because maybe the database connection is already gone, or maybe some useful object has been desctructed.

In the worst scenario our job is gone, and its payload is lost.

[rodrigopedra]

Precisely.

"The destructor method will be called AS SOON as there are no other references to a particular object, or in any order during the shutdown sequence".

MyJob::dispatch('foo')->onQueue('bar');

// no more references to the job right here, __destruct is already called

// ... other code before returning

Patch your local PendingDispacth with the code above, and test it out. You can even throw an exception from the __destruct or use dump(), as I did to test it:

// Illuminate\Foundation\Bus\PendingDispatch

    public function __destruct()
    {
        if (! $this->shouldDispatch()) {
            return;
        }

        try {
            dump(__METHOD__);

            if ($this->afterResponse) {
                app(Dispatcher::class)->dispatchAfterResponse($this->job);
            } else {
                app(Dispatcher::class)->dispatch($this->job);
            }
        } catch (\Throwable $exception) {
            if (method_exists($this->job, 'failed')) {
                $this->job->failed($exception);
            } else {
                throw $exception;
            }
        }
    }

And then add a dump() call just after the MyJob::dispatch()->onQueue()... call.

The one within the __destruct will be called before the following one.

[rodrigopedra]

On a fresh Laravel app, create a Job.

Use this code on ./routes/web.php

<?php

use App\Jobs\TestJob;
use Illuminate\Support\Facades\Route;

Route::get('/', function () {
    TestJob::dispatch()->onQueue('foo');

    dump(__FILE__);

    return 'bar';
});

This on the job:

<?php

namespace App\Jobs;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;

class TestJob implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    public function handle() {}

    public function failed($exception = null)
    {
        dump(__METHOD__);
    }
}

And then patch your PendingDispatch@__destruct method from your vendor folder (just for testing it out) with this:

public function __destruct()
{
    if (! $this->shouldDispatch()) {
        return;
    }

    try {
        if ($this->afterResponse) {
            app(Dispatcher::class)->dispatchAfterResponse($this->job);
        } else {
            app(Dispatcher::class)->dispatch($this->job);
        }

        dump(__METHOD__);
        throw new RuntimeException();
    } catch (\Throwable $exception) {
        if (method_exists($this->job, 'failed')) {
            $this->job->failed($exception);
        } else {
            throw $exception;
        }
    }
}

Finally, start your test server, and navigate to the home page. The output might be this:

"Illuminate\Foundation\Bus\PendingDispatch::__destruct" // vendor/laravel/framework/src/Illuminate/Foundation/Bus/PendingDispatch.php:207

"App\Jobs\TestJob::failed" // app/Jobs/TestJob.php:19

"/home/rodrigo/code/playground/pending/routes/web.php" // routes/web.php:9

bar

In summary, the destructor is called as soon no other references to the job instance are held. Before finishing the request.

I guess this fit your issue description: keep the fluent UI to customize the job when dispatching, while allowing the developer to handle any exceptions on the job level.

I am not sure if this code is sufficient, the job might need to be pushed to the failed_jobs, a new method might be created to handle this use-case.

As you said, the Queue component is such a sensitive part of the framework.

My 2 cents here was trying to find a solution that had the lowest impact on current usage, while fulfilling your requirements.

[driesvints]

Closing this issue because it's inactive, already solved, old or not relevant anymore. Feel to open up a new issue if you're still experiencing this.