Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedFeb 24, 2023
edited by nicolas-grekas
Loading

QA
Branch?6.3
Bug fix?yes
New feature?yes
Deprecations?yes (new method in interrface)
TicketsFix#48340
LicenseMIT
Doc PR

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:

  • By default, we exit, like PHP does by default onSIGINT,SIGTERM;
  • It's possible to tell via the event to not exit, by calling
    $event->abortExit();
  • It's possible to tell via the event to exit with a custom code, by calling
    $event->setExitCode($code);
  • It's possible, via theSignalableCommandInterface to tell via the command to
    not exit, by returningint|false fromhandleSignal() method
  • Fixed case when there is not event to dispatch at all, but the command register some

I 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

welcoMattic and dunglas reacted with thumbs up emojieexit reacted with hooray emoji
@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch fromaf0f5fe tofd64a13CompareFebruary 24, 2023 21:21
@lcobucci
Copy link
Contributor

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
Copy link
MemberAuthor

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.

@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch 2 times, most recently froma24c7e8 to3091710CompareFebruary 25, 2023 15:13
@fabpot
Copy link
Member

@lyrixx You can rebase this one (I've merged up the other 2 PRs).

@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch 2 times, most recently from47aee88 to35ccc92CompareFebruary 25, 2023 22:55
@lyrixx
Copy link
MemberAuthor

Rebase ✅

Copy link
Member

@chalasrchalasr left a 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
Copy link
MemberAuthor

lyrixx commentedFeb 26, 2023
edited
Loading

What bc break are you talking about? The new method in the interface? I should update the upgrade.md too!

@chalasr
Copy link
Member

Yes!

@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch from35ccc92 tof4fe1c1CompareFebruary 26, 2023 17:00
@lyrixx
Copy link
MemberAuthor

Fixed

@lyrixx

This comment was marked as resolved.

@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch fromf4fe1c1 to09dbb75CompareMarch 3, 2023 13:41
@lyrixx
Copy link
MemberAuthor

Thanks for the review!

I pushed a new version without the new method on the interface!

The new API is even simpler!

GromNaN reacted with hooray emoji

@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch 3 times, most recently from4c56899 to392f0ffCompareMarch 3, 2023 14:19
@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch 3 times, most recently from039c1a6 toe0b43d2CompareMarch 3, 2023 14:45
@nicolas-grekasnicolas-grekas changed the title[Console] Fix issue with signal handling[Console] AddgetExitCode() method toSignalableCommandInterfaceMar 3, 2023
@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch frome0b43d2 to3bbe3d1CompareMarch 3, 2023 15:15
@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch from3bbe3d1 to14a4f48CompareMarch 3, 2023 15:33
@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch from14a4f48 todab0517CompareMarch 3, 2023 15:42
@lyrixxlyrixxforce-pushed theconsole-fix-signal-handling branch fromdab0517 to1650e38CompareMarch 3, 2023 15:47
@lyrixxlyrixx changed the title[Console] AddgetExitCode() method toSignalableCommandInterface[Console] Add support for managing exit code while handling signalsMar 3, 2023
@nicolas-grekas
Copy link
Member

Thank you@lyrixx.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@GromNaNGromNaNGromNaN approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

+2 more reviewers

@phansysphansysphansys left review comments

@akuziaakuziaakuzia left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

[Console] Unexpected behavior on CONTROL-C

9 participants

@lyrixx@lcobucci@fabpot@chalasr@nicolas-grekas@GromNaN@phansys@akuzia@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp