Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Add support for managing exit code while handling signals#49529
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
4725a83 toaf0f5feCompareUh oh!
There was an error while loading.Please reload this page.
af0f5fe tofd64a13Comparelcobucci commentedFeb 24, 2023
This was the BC-break I referred to when I brought up the initial issue 😅 @lyrixx thanks for your work here, there are indeed many factors to take into account when tweaking signal handling. For my use case, the solution will work fine. It feels a bit cumbersome to modify the event state buuuut it's fine. It's really important to document the nuances and what's expected from each different use case 👍 |
lyrixx commentedFeb 25, 2023
Yes, it's not so cool to have to edit the event, but we must do it since we cannot now if the end user will stop / exit / return itself. So by default we should mimic PHP, and the user can opt-in for managing exit themself. |
a24c7e8 to3091710Comparefabpot commentedFeb 25, 2023
@lyrixx You can rebase this one (I've merged up the other 2 PRs). |
47aee88 to35ccc92Comparelyrixx commentedFeb 25, 2023
Rebase ✅ |
chalasr left a comment
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.
👍 for this. The CHANGELOG should mention the BC break though
lyrixx commentedFeb 26, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What bc break are you talking about? The new method in the interface? I should update the upgrade.md too! |
chalasr commentedFeb 26, 2023
Yes! |
35ccc92 tof4fe1c1Comparelyrixx commentedFeb 26, 2023
Fixed |
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
f4fe1c1 to09dbb75Comparelyrixx commentedMar 3, 2023
Thanks for the review! I pushed a new version without the new method on the interface! The new API is even simpler! |
4c56899 to392f0ffCompare039c1a6 toe0b43d2ComparegetExitCode() method toSignalableCommandInterfacee0b43d2 to3bbe3d1CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
3bbe3d1 to14a4f48Comparesrc/Symfony/Component/Console/Tests/SignalRegistry/SignalRegistryTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
14a4f48 todab0517Comparedab0517 to1650e38ComparegetExitCode() method toSignalableCommandInterfacenicolas-grekas commentedMar 3, 2023
Thank you@lyrixx. |
Uh oh!
There was an error while loading.Please reload this page.
Signal handling is hard! This commit fixes an issue introduced inbb7779d, see thiscomment for more information.
Symfony registers by default 4 signals, the most common ones:
SIGINT,SIGTERM,SIGUSR1, andSIGUSR2.When no signal handler are registered, the default behavior is to stop the
execution of the current script, to replication PHP default behavior.
That's why we had a call to exit(0), before
bb7779d and that's why it must be restored.
However, the concerns raised in the
issue are valid, and we
should provided a solution for this.
Now we have the following features:
SIGINT,SIGTERM;$event->abortExit();$event->setExitCode($code);SignalableCommandInterfaceto tell via the command tonot exit, by returning
int|falsefromhandleSignal()methodI think we have all we need now. I added more tests to ensure we don't break
anything in the future.
Sorry for the massive ping here, but this is bit like an a house of cards to
make it work in any case. I hope I didn't miss anything.
ping@akuzia@lcobucci@GromNaN@olegpro