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

term: remove interrupt handler on termios #31

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

enool
Copy link
Contributor

@enool enool commented Oct 4, 2022

On termios platforms, interrupt signals are not generated in raw mode terminals as the ISIG setting is not enabled. Remove interrupt handler as it does nothing for raw mode and prevents other uses of INT signal with this library.

This code seems to go back all the way to moby/moby#214 where signal handling was improved for monolithic docker repository. Raw mode -ISIG got reintroduced in moby/moby@3f63b878076, but the INT handler was left behind.

Signed-off-by: Sami Loone [email protected]

On termios platforms, interrupt signals are not generated in raw mode
terminals as the ISIG setting is not enabled. Remove interrupt handler
as it does nothing for raw mode and prevents other uses of INT signal
with this library.

This code seems to go back all the way to moby/moby#214 where signal
handling was improved for monolithic docker repository. Raw mode -ISIG
got reintroduced in moby/moby@3f63b878076, but the INT handler was left
behind.

Signed-off-by: Sami Loone <[email protected]>
@thaJeztah
Copy link
Member

@creack @tianon @cpuguy83 I could use some help on this one; does any of you have any ideas if there could be some obscure reason why this would still be needed? I realise it's been a long time since this was added, but perhaps there was some reason that I'm overlooking and that may still be relevant.

See docker/cli#3801 for more details (and I opened docker/cli#3849 to see if CI would explode, but all looks good on that front)

@creack
Copy link
Contributor

creack commented Nov 25, 2022

Exiting the parent process upon SIGINT is quite an unexpected behavior..., especially with code 1.

There is a difference between the terminal and the process. Even if the raw mode is set, while CTRL-C user input may not result in a SIGINT, something like docker stop or a manual kill will send a SIGINT directly to the process, which indeed docker/cli#3801 tries to do.
I would be in favor to remove that handler.

LGTM for Linux/BSD, however maybe it was put there for Windows? No clue.

That behind said, it would be a behavior change. Maybe some projects are relying on this.

@enool
Copy link
Contributor Author

enool commented Nov 25, 2022

Hey, thanks for taking a look.

however maybe it was put there for Windows? No clue.

Would this code run on Windows in some scenario? There is a split term_windows.go implementation and term.go has // +build !windows.

@thaJeztah
Copy link
Member

Would this code run on Windows in some scenario? There is a split term_windows.go implementation and term.go has // +build !windows.

Yes, this wouldn't affect Windows; the commit that introduced it (moby/moby@3f63b878076) was also long before we had support for Windows, so I don't expect it to be related.

Thank you both for the extra eyes / input! Let me go ahead and get this one in; I think this should be ok to get in, but (worst case) if we run into issues, we can always revert, or give it another look.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@thaJeztah thaJeztah merged commit c43b287 into moby:master Nov 28, 2022
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.

3 participants