Skip to content

[Console] Ensure terminal is usable after termination signal #61861

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Oct 2, 2025
Merged

[Console] Ensure terminal is usable after termination signal #61861

fabpot merged 1 commit into from
Oct 2, 2025

Conversation

@johnstevenson
Copy link
Contributor

@johnstevenson johnstevenson commented Sep 26, 2025

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #61732
License MIT

This PR introduces a new helper TerminalInputHelper that restores the terminal to its original state after it has been modified when reading using input. This ensures that the terminal is not broken when a terminating signal, like Ctrl-C, is received.

QuestionHelper.php disables icanon and echo when accepting input from a selection, and disables echo when accepting hidden input. If these are not restored before termination, the user's terminal can end up in a broken state.

Usage:

$inputHelper = new TerminalInputHelper($inputStream);

// Change terminal settings then wait for input before all input reads
$inputHelper->waitForInput();

// Read the input then call finish to restore terminal settings and signal handlers
$inputHelper->finish()

The helper creates its own signal handlers (for SIGINT, SIGQUIT, and SIGTERM) that restore the original terminal settings then call any original handler callback. If the original handler callback does not terminate the process then the current terminal settings are restored.

If there is no original signal handler callback and the signal's disposition is set to the default action (SIG_DFL), then that action is invoked by a posix_kill call.

The finish method restores the terminal settings and replaces the new signal handlers with the original ones.

@carsonbot carsonbot added this to the 6.4 milestone Sep 26, 2025
@carsonbot carsonbot changed the title Ensure terminal is usable after termination signal Ensure terminal is usable after termination signal Sep 26, 2025
@johnstevenson johnstevenson mentioned this pull request Sep 27, 2025
Open
@chalasr
Copy link
Member

chalasr commented Sep 28, 2025

Do you have a use case for using this helper in your own commands whether coming from a Symfony application or a reusable bundle? Elsewhere than in core I mean.

@johnstevenson
Copy link
Contributor Author

johnstevenson commented Sep 28, 2025

Do you have a use case for using this helper in your own commands...

Not at all. The helper is to fix the issues with Symfony/Console.

@johnstevenson
Copy link
Contributor Author

johnstevenson commented Sep 28, 2025

I've just noticed this issue #61852 (that has been closed). This PR fixes the problem described, which is having to hit Enter after a Ctrl-C to send the signal (when submitting hidden input).

@chalasr
Copy link
Member

chalasr commented Sep 28, 2025

Please inline back the logic into Application and QuestionHelper. Helpers in this namespace are part of Console's public API, but I can't think of a use case for this helper in userland either and I looking at it, I don't think it's worth it. Expanding the public API is a feature also while this PR is a bugfix targeting a branch that is in maintenance mode, so please do the minimum changes to fix the bug at hand. Thanks

@stof
Copy link
Member

stof commented Sep 29, 2025

Given the need for extra state in this input helper, I'm fine with using a separate class. But it should be @internal

@chalasr
Copy link
Member

chalasr commented Sep 29, 2025

The non-obvious name of that helper annoys me but probably fine if it’s internal. Works for me

nicolas-grekas
nicolas-grekas reviewed Sep 29, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's some nitpicking.

@valx76 valx76 mentioned this pull request Sep 29, 2025
Closed
@johnstevenson
Copy link
Contributor Author

johnstevenson commented Sep 29, 2025

The non-obvious name of that helper annoys me ....

I tried several before choosing the least annoying. Feel free to suggest an alternative.

@valx76
Copy link
Contributor

valx76 commented Sep 29, 2025

The non-obvious name of that helper annoys me ....

I tried several before choosing the least annoying. Feel free to suggest an alternative.

I can think of InputSignalHelper or SignalSafeTerminalInputHelper.
Don't know if it's any better, what do you think?

@carsonbot carsonbot changed the title Ensure terminal is usable after termination signal [Console] Ensure terminal is usable after termination signal Oct 1, 2025
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 1, 2025
Closed
@fabpot
Copy link
Member

fabpot commented Oct 2, 2025

Thank you @johnstevenson.

@fabpot fabpot merged commit 07a5ed4 into symfony:6.4 Oct 2, 2025
@johnstevenson johnstevenson deleted the sigbug branch October 2, 2025 09:20
@fabpot fabpot mentioned this pull request Oct 28, 2025
Merged
@fabpot fabpot mentioned this pull request Oct 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees

No one assigned

Labels

Bug Console Status: Reviewed

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

8 participants

@johnstevenson @chalasr @stof @valx76 @fabpot @nicolas-grekas @OskarStark @carsonbot