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

readline: only catch signals when using interact #303

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

JelteF
Copy link
Contributor

@JelteF JelteF commented Dec 22, 2021

When running ./hammerdbcli auto somescript.ctl on Linux it was
impossible to stop the benchmark by using Ctrl+C. This was pretty
annoying since it is the most common way to stop programs in a shell on
Linux. The reason was that the readline module was explicitly ignoring
SIGINT. This makes sense for interactive mode, but when using auto
there's no reason for this.

This change makes sure the signal capturing code is only executed once
TclReadLine::interactive is called.

When running `./hammerdbcli auto somescript.ctl` on Linux it was
impossible to stop the benchmark by using Ctrl+C. This was pretty
annoying since it is the most common way to stop programs in a shell on
Linux. The reason was that the readline module was explicitly ignoring
SIGINT. This makes sense for interactive mode, but when using `auto`
there's no reason for this.

This change makes sure the signal capturing code is only executed once
`TclReadLine::interactive` is called.
@sm-shaw
Copy link
Contributor

sm-shaw commented Jan 10, 2022

I have tested this on Linux and Windows and this is also recommended for acceptance.
This unifies the behaviour on Linux and Windows, as Ctrl+C on Windows already gives the option to terminate the workload, making the functionality more consistent on both platforms.
^CTerminate batch job (Y/N)? Y

@sm-shaw
Copy link
Contributor

sm-shaw commented Jan 11, 2022

Merging Pull Request as voted on by TPC-OSS subcommittee on 11th Jan 2022.

@sm-shaw sm-shaw merged commit 5b1ecc3 into TPC-Council:master Jan 11, 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.

2 participants