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

Change terminal mode to cbreak in execute_interactive and handle SIGINT #35602

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

renzepost
Copy link
Contributor

Related: #26186

Because the terminal mode was set to raw mode, Ctrl-C was not being interpreted as a SIGINT signal. Changing to cbreak mode will maintain most of the features of raw mode, except that keystrokes like Ctrl-C will be interpreted by the terminal and will interrupt.
Besides that, a handler for SIGINT is added so that the signal is simply passed to the subprocess (instead of causing a KeyboardInterrupt.)
This at least partially fixes #26186, because now hitting Ctrl-C will cause psql (and mysql) to interrupt any partial input.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member

potiuk commented Nov 13, 2023

Nice. Looks pretty resonable. World of low-level terminal interfaces is pretty intriguing

It's an interesting though why you need to send the signal to proc? It think we should simply ignore the signal rather than send it to the spawned process.

From what I understand how the Ctrl-C works is that it sends the signal to process group rather than to the main process.

When we create the child process (despite a comment - see below) it does not look like the process is created with a "create new process group" flag - so Ctrl-C should also be sent to the child process.

Of course we still do not want KeyboardInterupt Exception as you noticed, so I'd say we could likely ignore the signal rather than send it to the new process - otherwise the process will receive two signals one after the other.

Re the comment: we have this comment there:

        # use os.setsid() make it run in a new process group, or bash job control will not be enabled

So it looks like at least at some point of time there was a new process group created when spawning the process, but you cannot see it anywhere in the code. So maybe we shoudl also remove the comment while we are at it.

So I'd say just ignoring the signal would be a better approach.

@potiuk
Copy link
Member

potiuk commented Nov 13, 2023

Also one more comment here - when we set the signal to a new handler (or ignore which is equivalent), we should - at least in theory, save the previous handler and restore it in the finally clause. It does not matter likely in this case - because we are going to exit the db shell command in a moment. but purely thorethically, if anything happens between the finally and actual exit, then we will not be able to Ctrl-C it as usual without the handler restored.

@renzepost renzepost force-pushed the execute_interactive_tty_cbreak branch from 8ea6f88 to fa9dcf3 Compare November 14, 2023 11:46
@renzepost
Copy link
Contributor Author

Hi Jarek,

You were absolutely right! The subprocess has the same PGID, so I made some changes. Now the calling process simply ignores the SIGINT. I tested this with psql and mysql, and Ctrl-C works fine without the whole process shutting down with a KeyboardInterrupt.
Also, as you suggested, I restore the old SIGINT handler in the finally clause.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

This is cool ! Thanks for being responsive here :)!

LGTM

@potiuk potiuk merged commit 24aca11 into apache:main Nov 15, 2023
@renzepost renzepost deleted the execute_interactive_tty_cbreak branch November 15, 2023 08:56
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Nov 20, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad terminal handling of airflow db shell command with Postgres DB
3 participants