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

[11.x] HasMiddlewareHooks trait - Adds job middleware before, after, onFail Hooks #53764

Closed
wants to merge 7 commits into from

Conversation

TWithers
Copy link
Contributor

@TWithers TWithers commented Dec 5, 2024

This PR provides a trait that allows for hooks into existing job middleware at the point of failure or success with a simple api. It allows for modification of the Job itself before the middleware is executed, after it is successful (before $next($job) is called, or if it failed (failed or released). Not only does this allow for modifications, but also provides the needed ability to determine which middleware released/failed the job, allowing for enhanced monitoring and better job handling. It also provides the ability to skip the middleware if a condition is met.

Example Usage:

public function middleware(): array
{
    return [
        (new MyMiddleware)
            ->before(fn ($job) => Log::info('Job entering MyMiddleware', compact('job')))
            ->after(fn ($job) => Log::info('Job passed MyMiddleware. $next($job) will be called.', compact('job')))
            ->onFail(fn ($job) => Log::info('Job did not pass MyMiddleware. $next($job) will not be called.', [
                'job' => $job,
                'isReleased' => $job->job->isReleased(),
                'hasFailed' => $job->job->hasFailed(),
            ]))
            ->skipWhen($this->someCondition),
    ];
}

Background and usage:

This came about when trying to (1) diagnose and report which middleware was releasing jobs and (2) then trying to modify the job if it was released for a specific reason.

Some of the jobs were leveraging several middlewares, including RateLimited and WithoutOverlapping. When the jobs would release, there is no easy way to figure out why it was released unless we extended every middleware we were using and write a wrapper for the handle() method.

The thought occurred that it would be much simpler to handle these random scenarios with something like: (new RateLimitedWithRedis('rate-limiter'))->onFail(fn ($job) => $job->failReason = 'Rate Limiting')"

Quick example showcasing the issue:

// normal middlewares
class LoggerMiddleware
{
    public function handle($job, $next)
    {
        $next($job);

        if ($job->job->isReleased() || $job->job->hasFailed()) {
                Log::info('Job failed or released... but why?', ['job' => $job]);
        }
    }
}
class MiddlewareA
{
    public function handle($job, $next)
    {
        if (rand(0, 1)) {
            $next($job);
        } else {
            $job->release(5);
        }
    }
}

class MiddlewareB
{
    public function handle($job, $next)
    {
        if (rand(0, 1)) {
            $next($job);
        } else {
            $job->release(5);
        }
    }
}

// normal job
class Job implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable;
    
    public function middleware()
    {
        return [new LoggerMiddleware, new MiddlewareA, new MiddlewareB];
    }
    
    public function handle()
    {
        dump('handled');
    }
}

When Job::dispatch() is called, there is a 50% chance it passes MiddlewareA, and if it passes, another 50% chance it passes MiddlewareB. As it stands, there is no way to know whether it was MiddlewareA or MiddlewareB or the job handle itself that called release().

The current solution is to extend the middleware, but that doesn't feel right, especially when different jobs might want to do different things with the same middleware.

Order of operations:

class MiddlewareA
{
    use HasMiddlewareHooks;

    public function handle($job, $next)
    {
        Log::info('MiddlewareA initialized');
        
        if (rand(0, 1)) {
            $next($job);
        } else {
            $job->release(5);
        }

        $job->status = 'MiddlewareA finished';
    }
}

class MiddlewareB
{
    use HasMiddlewareHooks;

    public function handle($job, $next)
    {
        Log::info('MiddlewareB initialized');
        
        if (rand(0, 1)) {
            $next($job);
        } else {
            $job->release(5);
        }

        $job->status = 'MiddlewareB finished';
    }
}

// normal job
class Job implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable;
    
    public function middleware()
    {
        return [
            (new MiddlewareA)
                ->before(fn ($job) => Log::info('MiddlewareA before'))
                ->after(fn ($job) => Log::info('MiddlewareA after'))
                ->onFail(fn ($job) => Log::info('MiddlewareA failed')),
            (new MiddlewareB)
                ->before(fn ($job) => Log::info('MiddlewareB before'))
                ->after(fn ($job) => Log::info('MiddlewareB after'))
                ->onFail(fn ($job) => Log::info('MiddlewareB failed')),
        ];
    }
    
    public function handle()
    {
        Log::info('Job Handled');
    }
}

Logs when both middlewares pass:

MiddlewareA before
MiddlewareA initialized
MiddlewareA passed
MiddlewareB before
MiddlewareB initialized
MiddlewareB passed
JobHandled
MiddlewareB finished
MiddlewareA finished

Logs when MiddlewareA fails:

MiddlewareA before
MiddlewareA initialized
MiddlewareA failed
MiddlewareA finished

Logs when MiddlewareB fails:

MiddlewareA before
MiddlewareA initialized
MiddlewareA passed
MiddlewareB before
MiddlewareB initialized
MiddlewareB failed
MiddlewareB finished
MiddlewareA finished

Additional Notes

If the before() hook releases the job, fails the job, or returns false, the middleware is aborted before it is ever executed. Using the example above, and changing MiddlewareA's before() to: before(fn ($job) => false), the logs would only show:

MiddlewareA before

The skipWhen or skipUnless will skip the handle() of the middleware and simply call $next($job), but it will not skip initialization of the middleware. So queries and modifications in the __construct() would still be executed.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

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

Successfully merging this pull request may close these issues.

2 participants