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

[5.4] Fix for withOverlapping, the scheduled task would not resume #19419

Merged

Conversation

tschallacka
Copy link
Contributor

Fix for withOverlapping, the scheduled task would not resume if an error occurred.
This adds a last resort handler to clean up the mutex file if the application ended unexpectedly due to an fatal error.

Without this last ditch cleanup on application exit, the file will keep existing, and will not be cleaned up until the call ->withoutOverlapping() is removed from the scheduled task.

      $schedule->call(function() {
                   
            $worker = new Worker();
            $worker->workAlready();
        })
        ->name('automatic_email_sending')
        ->withoutOverlapping()
        ->when(function() {
            
            // weekdays 7:45 - 8:30
            // weekends 8:45 - 18:30
            $now = Carbon::now();
        
            if($now->dayOfWeek == Carbon::SUNDAY) {
                return false;
            }
        
            $start = $now->copy()->setTime(7,45, 0);
            $end = $now->copy()->setTime(19,30, 0);
        
            $random = new Random($now->diffInDays(Carbon::createFromDate(2017,1,1)));
        
            $int = $random->nextInt(0,45);
            $start->addMinutes($int);
        
            if ($now->isWeekend()) {
                $start->addHour();
                $end->subHour();
            }
            
            return $now->between($start, $end);
        });

-- Worker.php
    namespace Tschallacka\Workers;
    /**
      * cause a fatal error, Class Tschallacka\Workers\Carbon not found
      **/
    class Worker {
         public function workAlready() {
              $now = Carbon::now()
         } 
    }

mdibbets and others added 2 commits May 31, 2017 15:43
…ror occurred. This adds a last resort handler to clean up the mutex bit if the application ended unexpectedly
@tillkruss tillkruss changed the title Fix for withOverlapping, the scheduled task would not resume [5.4] Fix for withOverlapping, the scheduled task would not resume Jun 1, 2017
@taylorotwell taylorotwell merged commit 2041d09 into laravel:5.4 Jun 7, 2017
@bbashy
Copy link
Contributor

bbashy commented Jun 7, 2017

My server froze the other day and one queue schedule didn't work after boot. Will this help for that? I had to manually clear it.

@tschallacka
Copy link
Contributor Author

It should, i had the same issues when developping a task that had an error. I got so annoyed by removing the withoutOverlapping part everytime so i investigated.

This will always remove the mutex file if php ends normally. Even when it was by error, exception or voodoo.(slaughtered a chicken to test the last one)

Only cases where it wont is if php is killed by a kill command from the system or powerloss/abrubt system power cycle. Then you might have to manually clean up.

@tschallacka
Copy link
Contributor Author

On second thought, with a server freeze php might not have terminated normally, so it wont fix it for that case. Depends fully on the circumstances

@bbashy
Copy link
Contributor

bbashy commented Jun 7, 2017

@tschallacka Thanks for clarification on that. When this is released, I'll do some tests and see.
I need to use withoutOverlapping with this task (just incase it does take longer than a minute).

I was going to suggest some sort of timescale for withoutOverlapping so it would ignore it if it's past that time. Not very familiar with scheduling code in the framework so not sure how viable that is.

@tschallacka
Copy link
Contributor Author

I would rather suggest creating an artisan command something like

php artisan schedule:recover

That will remove old task locks.

And have a windows task/system event execute that in case of webserver service restart.
That way all cases can be covered

@bbashy
Copy link
Contributor

bbashy commented Jun 7, 2017

Makes sense 👍

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.

4 participants