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

[9.x] Raise ScheduledBackgroundTaskFinished event to signal when a run in background task finishes #37373

Closed
wants to merge 1 commit into from

Conversation

aaronlp
Copy link
Contributor

@aaronlp aaronlp commented May 15, 2021

PR #29888 added excellent new events ScheduledTaskStarting and ScheduledTaskFinished which are fired before and after a scheduled task runs.

However, where the scheduled task is set to run in background, ScheduledTaskFinished fires immediately because we push the task off to a separate process. This makes it difficult to run any relevant activities after a background task finishes, without having to hack on to the after callbacks.

This PR adds a new event ScheduledBackgroundTaskFinished which we fire within the schedule:finish command which allows us to reliably determine when a background task actually completed without having to attach after callbacks to each scheduled task.

We're adding a brand new event class and firing it without changing any additional functionality but since we're introducing a new argument to the handle method on the schedule:finish command, so targetting master branch for this.

@GrahamCampbell GrahamCampbell changed the title new ScheduledBackgroundTaskFinished event to signal when a run in background task finishes [9.x] Raise ScheduledBackgroundTaskFinished event to signal when a run in background task finishes May 15, 2021
@taylorotwell
Copy link
Member

Personally I would just use Container::getInstance()->make(Dispatcher::class) to get the event dispatcher - that way this can be sent to 8.x as a non-breaking change.

@aaronlp
Copy link
Contributor Author

aaronlp commented May 15, 2021

Ok thanks. Will reopen my 8.x PR #37367 and make that change. Thanks

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.

2 participants