Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix missing dispatchWorkerStoppedEvent afterSIGINT orSIGTERM#52123
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $this->worker->stop(); | ||
| $this->eventDispatcher?->dispatch(newWorkerStoppedEvent($this->worker)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't look necessary to me. The event should already be dispatched at the end of theWorker::run() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Worker::run() is not called in the case of SIGINT or SIGTERM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure that's still the case after the fix from#52080?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i will check that
WorkerStoppedEvent afterSIGINT orSIGTERMWorkerStoppedEvent afterSIGINT orSIGTERMWorkerStoppedEvent afterSIGINT orSIGTERMOskarStark commentedOct 20, 2023
Any news@Chris53897 ? |
Chris53897 commentedOct 20, 2023
Sorry for late response. I will close this PR. |
Related to#52077
In my point of view this Event should be dispatched here.
But maybe there are reasons for not doing this in the past.
I am not sure if bugfix or new feature.