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

batches: handle interrupt signals in batch preview #793

Closed

Conversation

BolajiOlajide
Copy link
Contributor

Closes #775

Test plan

Confirm Ctrl + C terminates the src batch preview -f - command.

@BolajiOlajide BolajiOlajide requested a review from a team June 21, 2022 15:56
@BolajiOlajide BolajiOlajide self-assigned this Jun 21, 2022
Copy link
Contributor

@Piszmog Piszmog left a comment

Choose a reason for hiding this comment

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

Thank you!

@eseliger
Copy link
Member

wondering how you found that 😬

@@ -533,7 +534,7 @@ func checkExecutable(cmd string, args ...string) error {
func contextCancelOnInterrupt(parent context.Context) (context.Context, func()) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe that name is not 100% accurate anymore.

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@BolajiOlajide
Copy link
Contributor Author

BolajiOlajide commented Jun 22, 2022

Did a bit more digging into this and the fix doesn't quite work as expected.

Sending Ctrl + C sends the SIGINT signal, however for some reason it sends the SIGTERM signal when the terminal is frozen and the terminal wants to force quit the current session.
However, once the execUI variable that holds an instance of the TUI struct is instantiated, it doesn't honor the context as it has no idea of it, thus ignoring the call to cancel from the created context.WithCancel.

I updated the goroutine that handles the signal to

go func() {
	select {
	case <-c:
		ctxCancel()
		os.Exit(0)
	case <-ctx.Done():
	}
}()

However, this means we always exit automatically after canceling the cotext and I'm not sure this is the right way to go about this. Does anyone have any insight as to the best approach to this?

cc @LawnGnome @Piszmog @eseliger

mrnugget added a commit that referenced this pull request Jun 23, 2022
This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.
@mrnugget
Copy link
Contributor

I got kinda nerd-sniped reading through this... My first thought was: "isn't there a way to abort an io.ReadAll call when a context is cancelled?" and then I found this comment here golang/go#20280 (comment) and quickly copy&pasted&hacked together this: #794

It seems to do what it should do, but not sure about unintended side-effects etc. Just wanted to possibly add something to conversation 🙂

screenshot_2022-06-23_10 26 51@2x

@mrnugget
Copy link
Contributor

(Also, have to mention this: the reading stops when you hit Ctrl-D, because Ctrl-D sends the EOF value to the application, which means ends the input reading with an empty file. So, another way to avoid the ugly error would be to print to the user "Hit Ctrl-d to abort" or something, when we detect that input is os.Stdin)

@BolajiOlajide
Copy link
Contributor Author

Cancel reading from stdin on Ctrl-C

Thanks @mrnugget . Testing it out now and reading the attached issue.

@BolajiOlajide
Copy link
Contributor Author

Closing this for now. I'll work with Thorsten on his fix for this problem.

BolajiOlajide pushed a commit that referenced this pull request Jul 7, 2022
This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.
BolajiOlajide added a commit that referenced this pull request Jul 7, 2022
* Cancel reading from stdin on Ctrl-C

This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.

* Add comments and remove interface

* update changelog

Co-authored-by: BolajiOlajide <[email protected]>
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* Cancel reading from stdin on Ctrl-C

This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in #793 (comment). Not sure if it has unintended side-effects.

* Add comments and remove interface

* update changelog

Co-authored-by: BolajiOlajide <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control+C does not work when reading batch spec as standard input
5 participants