-
Notifications
You must be signed in to change notification settings - Fork 11.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[9.x] Refactor scheduled event lifecycle #39539
[9.x] Refactor scheduled event lifecycle #39539
Conversation
@inxilpro This works on 8.x but doesn't work on this PR. The "after" log is never written: That makes me uneasy about this entire PR. 😬 That there could be other subtle (or even not so subtle bugs like this one) present. |
@taylorotwell yeah—it's tricky since there aren't really any tests for the scheduler. Let me see what I can come up with. I'll close for now and re-submit if I feel like I have a better way to ensure it doesn't break anything. |
@taylorotwell the issue with doing (Another solution would be to continue to ignore it, but that seems more like a bug to me.) |
Thanks for contributing to Laravel! ❤️ |
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
.