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

[Proposal] Job middleware #1356

Open
hubertnnn opened this issue Oct 14, 2018 · 13 comments

Comments

@hubertnnn
Copy link

commented Oct 14, 2018

Currently if you need to limit your job execution (eg. becouse of API limits etc.) you have to write an ugly code like this (from laravel documentation):

public function handle() 
{
    Redis::throttle('key')->allow(10)->every(60)->then(function () {
        // Job logic...
    }, function () {
        // Could not obtain lock...
        return $this->release(10);
    });
}

It is even worse if you need more than 1 "wrapper"

public function handle() 
{
    Redis::throttle('key')->allow(10)->every(60)->then(function () {
        Redis::funnel('key')->limit(1)->then(function () {
            // Job logic...
        }, function () {
            // Could not obtain lock...
            return $this->release(10);
        });

    }, function () {
        // Could not obtain lock...
        return $this->release(10);
    });
}

I suggest addition of middleware system for jobs similar to controllers, so that you could define your jobs much easier:

// app/Jobs/MyJob.php
class MyJob
{
    public $middleware = ['throttle:10,60', 'funnel:1'];

    public function handle()
    {
        // Job logic...
    }
}

// app/Jobs/Middleware/ThrottleMiddleware.php
class ThrottleMiddleware
{
    public function handle($job, Closure $next, $allow, $every)
    {
        Redis::throttle('key')->allow($allow)->every($every)->then(function () {
            $next($job);
        }, function () {
            // Could not obtain lock...
            return $this->release(10);
        });
    }
}

// app/Console/Kernel.php
protected $jobsMiddleware = [
    'throttle' => \App\Jobs\Middleware\ThrottleMiddleware::class,
    'funnel' => \App\Jobs\Middleware\FunnelMiddleware::class,
];

@danrichards

This comment has been minimized.

Copy link

commented Dec 7, 2018

A separate use case where job middleware would be useful, is modifying environmental conditions. This is something I've built route middleware around that would be lovely to expose in our job environment.

public $middleware = [
    'max_execution_time:60',
    'memory_limit:1024M',
];
@viezel

This comment has been minimized.

Copy link

commented May 21, 2019

Agreed. The same can be said about artisan commands. The ability to have middlewares supported in the console/Kernal would be very beneficial.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

I think this is a pretty interesting proposal and I think I would have used this feature before if it existed. It's actually pretty simple to implement I think... I wouldn't use that syntax though. For maximum flexibility and power I would just make it a normal PHP method on the job:

image

Alternatively allow specifying middleware on the dispatch:

image

@viezel

This comment has been minimized.

Copy link

commented Jul 31, 2019

yes please @taylorotwell 👏 . Can we get it into 6.0 release?

@olivernybroe

This comment has been minimized.

Copy link

commented Jul 31, 2019

Using @taylorotwell proposal doesn't have to disallow the other syntax.
It could be like with many other properties in Laravel.
So you can set it directly in the property or override the method for full control.

@LasseRafn

This comment has been minimized.

Copy link

commented Jul 31, 2019

I'm so down for this!

@librevlad

This comment has been minimized.

Copy link

commented Jul 31, 2019

Sounds very interesting and clean! I'd probably not use Console Kernel to define those.
P.S. Can we also get job logging on that note? Maybe to Horizon? Really need something to debug jobs, especially when writing something that relies on 3rd party API or web scraping.

@michaeldyrynda

This comment has been minimized.

Copy link

commented Jul 31, 2019

Pretty clean; I definitely prefer the public method approach over a property, because it gives you a little more flexibility in terms of extension than a property 👍

@histonedev

This comment has been minimized.

Copy link

commented Aug 1, 2019

Middleware looks like a great idea. Currently I write this logic in Parent Job class's handle method and call a perform method from there, which is then overridden in child job classes. A controller like middleware will be a far cleaner solution.

@SjorsO

This comment has been minimized.

Copy link

commented Aug 1, 2019

What are some other real world examples for job middleware apart from throttling? Edit: or funneling

@histonedev

This comment has been minimized.

Copy link

commented Aug 1, 2019

@SjorsO In our case, the jobs are user based. Each job contains a user object and does some processing on that user. We dont want overlaps. i.e. one particular job should run only once per user. For this, we set a user_id based redis lock and upon start, we check if the current job type is already running for this user.

@matthew-muscat

This comment has been minimized.

Copy link

commented Aug 2, 2019

This is also something we would utilise if it's available — it's particularly useful for repetitive rate limiting / throttle operations, but i can see this also being useful for jobs that require a common set of functionality to occur prior to the job starting.

Would also be great if we could add terminate actions for flexibility and consistency with the HTTP middleware implementation.

@DarkGhostHunter

This comment has been minimized.

Copy link

commented Aug 6, 2019

Okay, seeing the code there must be some new additions:

1) The PendingDispatch should have the middleware function.
2) These middlewares would be also serialized as part of the dispatchable.
3) The middlewares should be instanced before handling the Job.
4) The Queue Worker would pass through a Pipeline class, and finally hit the Job handle.

Should that be enough?

Was implemented by Taylor a while ago:

laravel/framework#29391

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.