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.

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

lua: avoid tarantool console quit if sigint signal sending #6633

Merged
merged 1 commit into from Mar 17, 2022
Merged

lua: avoid tarantool console quit if sigint signal sending #6633

merged 1 commit into from Mar 17, 2022

Conversation

vr009
Copy link
Contributor

@vr009 vr009 commented Nov 22, 2021

Tarantool console quits if you type Ctrl+C.

This patch fixes console behavior on typing Ctrl+C. The patch is intended to make user experience of using tarantool console better.

Relates to #2717

@ligurio
Copy link
Member

ligurio commented Nov 24, 2021 •
edited

Regarding commit message:

lua: avoid tarantool console quit if sigint signal sending

Tarantool console quits if you type Ctrl+C.

This patch fixes console behavior on typing Ctrl+C. The patch is intended to make user experience of using tarantool console better.

Length of lines in commit message body is limited by 72 symbols. See Developer's guideline.

Relates to #2717

The proper keyword here is 'Fixes' with colon. See Developer's guideline.

@ligurio
Copy link
Member

ligurio commented Nov 24, 2021

I see that many jobs are failed on CI. Before merge all them must be passed.

I run Tarantool in interactive mode and send SIGINT to it (kill -INT pid) and input was moved to a new line:

sergeyb@pony:~/sources/MRG/tarantool/build$ ./src/tarantool 
Tarantool 2.10.0-beta1-153-g71a789f07
type 'help' for interactive help
tarantool> 
tarantool>

Looks like a correct behavior.
Then I enter random symbols to console and send SIGINT again:

tarantool> -- Hello, Tarantool!
tarantool>

Input was moved to a new line again.

Let's compare behavior with ftp.
With empty line:

ftp> ^C
ftp> ^C

With line that contains entered symbols:

ftp> ftp is a file transfer protocol^C
ftp>

Do you see a difference? I think we should print '^C' too.

ligurio
ligurio requested changes Nov 24, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
test/app-tap/gh-2717-no-quit-sigint.test.lua Show resolved Hide resolved
@Totktonada
Copy link
Member

Regarding ^C: it seems, it is controlled by stty -echoctl and is out of application's responsibility. Anyway, it is interesting why it does not work for us exactly like for bash.

@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch from 3572eb0 to db06163 Compare November 25, 2021 23:54
Totktonada
Totktonada reviewed Nov 26, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
Totktonada
Totktonada reviewed Nov 26, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
Totktonada
Totktonada reviewed Nov 26, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
Totktonada
Totktonada reviewed Nov 26, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
Totktonada
Totktonada reviewed Nov 26, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch 4 times, most recently from 63dccbf to 8d0c446 Compare December 6, 2021 11:00
@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch 2 times, most recently from eea3a89 to 9c81969 Compare December 12, 2021 20:39
@vr009
Copy link
Contributor Author

vr009 commented Dec 12, 2021

Let's compare behavior with ftp. With empty line:

ftp> ^C
ftp> ^C

With line that contains entered symbols:

ftp> ftp is a file transfer protocol^C
ftp>

Do you see a difference? I think we should print '^C' too.

It is a good idea. This behavior is added with latest commit.

@vr009
Copy link
Contributor Author

vr009 commented Dec 12, 2021

Regarding ^C: it seems, it is controlled by stty -echoctl and is out of application's responsibility. Anyway, it is interesting why it does not work for us exactly like for bash.

I guess that bash and Tarantool console just have different SIGINT handlers, that is why the behavior can be different. So if we need to print anything in console after any signal we should add this behavior to handler manually.

@vr009 vr009 requested a review from ligurio December 14, 2021 11:18
@Totktonada
Copy link
Member

There are comments without your answer opened three weeks ago. Without response from an author of a patch it is hard to track progress: one need to re-verify each comment manually during review. It is responsibility of the author of the patch.

ligurio
ligurio requested changes Dec 14, 2021
View reviewed changes
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

Waiting for resolving all open comments.

Totktonada
Totktonada reviewed Dec 14, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
Totktonada
Totktonada reviewed Dec 14, 2021
View reviewed changes
src/main.cc Outdated Show resolved Hide resolved
@vr009 vr009 requested review from locker and Totktonada March 16, 2022 08:22
@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch 3 times, most recently from c9e6d2e to 9ec6db0 Compare March 16, 2022 09:15
locker
locker previously approved these changes Mar 16, 2022
View reviewed changes
@locker locker assigned Totktonada and unassigned vr009 Mar 16, 2022
Totktonada
Totktonada previously approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

LGTM.

@Totktonada Totktonada removed their assignment Mar 16, 2022
@Totktonada Totktonada added the full-ci Enables all tests for a pull request label Mar 16, 2022
@vr009 vr009 added full-ci Enables all tests for a pull request and removed full-ci Enables all tests for a pull request labels Mar 16, 2022
@vr009 vr009 dismissed stale reviews from Totktonada and locker via 1dfa887 March 16, 2022 20:14
@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch 3 times, most recently from e5abfa6 to 9683b1c Compare March 16, 2022 21:37
@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch from 9683b1c to fff8785 Compare March 16, 2022 23:58
@vr009 vr009 requested a review from Totktonada March 17, 2022 06:38
@vr009
lua: fix SIGINT handling
4baadc8 
Tarantool console quits if you type Ctrl+C.
This patch fixes console behavior on sending SIGINT.
Console discards the current input on typing Ctrl+C
and invites user to the new line.

In daemon mode the process will exit after receiving SIGINT.

Test gh-2717 should be skipped on release build, cause it
uses error injection which is enabled only on debug mode.

Fixes #2717

@TarantoolBot document
Title: Use Ctrl+C to discard the input in console

The signal SIGINT discards the input in console mode.
When tarantool executes with -e flag or runs as a daemon, SIGINT
kills the process and tarantool complains about it in log.
@vr009 vr009 force-pushed the vr009/gh-2717-no-quit-sigint branch from fff8785 to 4baadc8 Compare March 17, 2022 10:26
@locker locker merged commit a8b3b26 into master Mar 17, 2022
@locker locker deleted the vr009/gh-2717-no-quit-sigint branch March 17, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Totktonada Totktonada Totktonada approved these changes

@locker locker locker approved these changes

@ligurio ligurio ligurio left review comments

Assignees
No one assigned
Labels
full-ci Enables all tests for a pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't quit on SIGINT in console
5 participants
@vr009 @ligurio @Totktonada @coveralls @locker