laravel / ideas

Issues board used for Laravel internals discussions.
940 stars 28 forks source link

[Proposal] (development) scheduler from artisan #941

Open TvL2386 opened 6 years ago

TvL2386 commented 6 years ago

Hey guys,

I'm just learning laravel the last two weeks (Merry Christmas!). I have created a laravel app which I run in docker containers (on Kubernetes).

The problem I had was that my docker containers did not have cron. Also running a container to only execute cron for my single php artisan schedule:run sounded like overkill imho.

So, I created a development scheduler which I can invoke with: php artisan schedule:daemon code: https://gist.github.com/TvL2386/67fe243079cbdb6c3e7d0c0a11509782

If you copy that code to app\Console\start_scheduler.php you have the artisan option. It runs great in my Docker environment and it makes sure I don't have to adjust my local crontab every time I decide to work on this project.

One thing that you have to keep in mind, is that if a job takes longer than 60 seconds, you are gonna miss the next minute. Cron does not have this limitation because it will (probably, I don't think laravel has lock files?) execute a new run on the next minute mark. I solved this by doing nothing but queueing jobs (in redis). The workers will handle the scheduled jobs.

Instead of executing php artisan schedule:run, I wanted to call the code directly internally, but failed. I got to:

use Illuminate\Console\Scheduling\Schedule;
use Illuminate\Console\Scheduling\ScheduleRunCommand;
$schedule = app(Schedule::class);
(new ScheduleRunCommand($schedule))->handle();

instead of php artisan schedule:run, but that throws an exception:

PHP Error:  Call to a member function isDownForMaintenance() on null in /myapp/vendor/laravel/framework/src/Illuminate/Console/Scheduling/Event.php on line 265

I wanted to simply create a pull request, but I'm having problems getting through the laravel code to see where I should put it... I'm new to php and laravel...

Kind regards!

PS: I mentioned my schedule:daemon here as well: https://github.com/laravel/internals/issues/908

sisve commented 6 years ago

The problem I had was that my docker containers did not have cron. Also running a container to only execute cron for my single php artisan schedule:run sounded like overkill imho.

I disagree. This is the single decision you made that required you to build this thing. Another container and the built-in stuff would have worked for you. Even installing cron on the existing containers would have been enough.

One thing that you have to keep in mind, is that if a job takes longer than 60 seconds, you are gonna miss the next minute.

So you built something that is worse than the existing solution.

I don't think laravel has lock files?

This indicates that you do not know enough about the current scheduler implementation to be attempting to write this hack.

I wanted to simply create a pull request, [...]

Don't PR this. Either use the standard stuff that comes with the framework, or use your own solution privately. Don't share weird stuff. This isn't the comments field on php.net where people share weird code snippets with weird requirements.

TvL2386 commented 6 years ago

Hey @sisve,

I did try to run my container with cron installed, but then you need to strip any cronjobs that are installed by default and you also need to make sure all ENV variables are present when invoked from cron.

Other reasons why I would want this:

I agree that the current state of my code with the current limitations should not be merged as is.

Wow... No need to be so denigrating...

mbkv commented 6 years ago

I'm kind of interested in this as well. I did some code reading and found out the mechanism on how schedule:run works.

Long story short, Laravel relies on a dependency named CronExpression, which all it does is check if the current time without seconds matches with the next run date (refer to the "isDue" function here)

return $this->getNextRunDate($currentDate, 0, true)->getTimestamp() == $currentTime;

In which case there's a slight issue with your code as is. It breaks if we have the assumption that there would be a task that spans 2 minutes, as the task that happens the minute after won't be created. If we do want to send something to the shell, we could change the command to this

php artisan schedule:run &

But then we get into the trouble of assuming that php is always going to be in $PATH. And in Windows machines, this isn't the case

What would be better (but not perfect) is if we have a Job that gets queued every minute. But it will still not work exactly the same because we get into the situation of backlogging the queue in rate limited tasks. PHP processes can happen asynchronously, while a queue just waits until one is finished before starting the next

Maybe we can use a multithreaded solution? But honestly I don't know how that would conflict with tasks creating threads of their own

I think this feature should be seriously considered, but not at the current state. ~I'd be fine with jobs/queue idea. But people might need a asynchronous process instead.~ Scratch that, it wouldn't work in rate limiting tasks. Multithreaded tasks seem to be the only hope. Maybe Laravel can eventually provide a Thread facade

joshmanders commented 6 years ago

@sisve

So you built something that is worse than the existing solution.

This indicates that you do not know enough about the current scheduler implementation to be attempting to write this hack.

Don't PR this. Either use the standard stuff that comes with the framework, or use your own solution privately. Don't share weird stuff. This isn't the comments field on php.net where people share weird code snippets with weird requirements.

There's no need to be rude. He may not be Taylor or you or Jeffrey in terms of skills, but doesn't mean his views and opinions are useless.

This response and attitude makes the community seem hostile and unwelcoming. Keep that attitude in r/php.

mfn commented 6 years ago

I'll try to summarize what I read here mixed with my own thoughts:

What is looked for

Seems to me that an endless loop+ forking could do the job.

I would highly suggest to prefer forking over threading; the latter will only cause all sorts of troubles, especially with 3rd party code/libraries.

There's of course also the case of event loops, but I anticipate this will cause even more troubles as all code you want to run with it needs to be "event-loop-able", which only few libraries are and I'm not even talking about your application code here.

There are a few caveats to watch out for though

  1. sleep(60) likely won't cut it. If your process has slow parts or the system running it gets slowed down, the process will drift and may jump over minutes. I'm not an expert on this topic, a naive way forward could be to just have the process sleep 30s and then ensure only to kick in after full minutes have passed. Maybe though the due-checking code is good enough and will cater for this, can't say
  2. you need to fork for every scheduled job which is run in a given minute because you need to make absolutely sure nothing blocks the scheduler loop. Depending on how many jobs you have, that could me a lot (or likely can at certain times like full hours, midnight, etc.). Forking also costs time and may be a factor for 1)
  3. Be careful with service providers creating singleton instances eagerly; these singletons will leak into every forked process and may not necessarily what you want

And we haven't talked about the details

Like everything else, it needs a proper implementation

Closing thoughts

A few things here are very similar to how the Worker-concept in Laravel works. I guess that's why queuing was brought up. But a queue won't work properly unless you ensure you always have enough spare workers so they immediately pick up the jobs, etc.

Also I don't think any implementation will every be accepted directly into core:

  1. introduced additional complexity
  2. for a minority of developers (end users?); at least right now Even though issue mentions "developers", I bet Docker-ish people will definitely try to use this in production too ;-)
  3. can perfectly be a made as a stand-alone package It may not necessarily be able to re-use all of the existing code, some parts may require duplication.

Happy developing though 🎅


(PS: I'm not interested in developing this but I wrote similar things with Laravel in the past and included some of the experiences in this summary)

joelharkes commented 4 years ago

To be honest what would be much nicer is to have a direct laravel scheduler scheduling kubernetes jobs: the tasks aren't run in separate process but in separate but identical containers and settings.

One day I might build this ;)