[9.x] Refactor scheduled event lifecycle #39539
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is building on the work in #39498. I'm submitting it against
9.x
because I've changed some method names. This would only affect applications that have custom versions ofScheduling\Event
orScheduling\CallbackEvent
.This PR splits scheduled event execution into three parts:
start
: Run before callbacks (removing mutex on exception)execute
: Execute the command or run the callbackfinish
: Set the exit code, run after callbacks, remove mutexBy doing so, we can improve the handling of mutex's and minimize the difference between foreground/background execution and across regular and callback events:
runCommandInBackground
andrunCommandInForeground
because start/finish are considered separate actions in both cases.callBeforeCallbacks
andcallAfterCallbacks
and easily perform exception handling around them.callAfterCallbacks
vscallAfterCallbacksWithExitCode
— thefinish
method is used in foreground and background execution in exactly the same way.CallbackEvent
can be removed now thatexecute
is its own discrete step. All the mutex handling and callback logic stays the same and the only real difference is the execution of the event.It also addresses a potential race condition in
CallbackEvent
. Because we register a shutdown function to remove the mutex:framework/src/Illuminate/Console/Scheduling/CallbackEvent.php
Lines 69 to 73 in 032c69a
And we also remove the mutex in a
finally
block:framework/src/Illuminate/Console/Scheduling/CallbackEvent.php
Lines 87 to 91 in 032c69a
...it's possible that the mutex will be removed twice by the same process. I tested this theory by spawning 100 PHP processes concurrently that created and deleted a lock file using both
register_shutdown_function
andtry
/finally
. I then logged the results to determine if the lock file was inconsistently removed by both methods:Show test scripts
Test Script
Runner
You can see from the logs that the lock file was sometimes removed by the
try
/finally
code and at other times was removed inside theregister_shutdown_function
code:Show log results
Log File
As a result, I feel like this refactor is doubly valuable, as it mitigates the race condition while also minimizing the difference between
Event
andCallbackEvent
.