WIP handle ctrl-z nicely to fix #48 #53

asdf · 2019-10-10T07:44:40Z

asdf commented

2019-10-10 07:44:40 +00:00

This is a first go at fixing this issue.

Most of this was taken from your suggestion in #48 and this cool blog post

It only handles SIGCONT, which is the signal sent to the application when you bring it back to the foreground. I don't think we need to worry about the initial SIGINT, which is when you press CTRL+Z.

It works like this:

signal.Notify captures syscall.SIGCONT to channel c
function handleSIGCONT takes this channel as an argument, and is started as a goroutine. It waits for the signal before doing anything.
When SIGCONT occurs, handleSIGCONT fixes the display, then starts another instance of handleSIGCONT before finishing.
This repeats each time SIGCONT is captured

A few things:

This might fit in an init() function
Is the recursion appropriate? Is it readable?
I am not sure how to fix the display. I chose those functions and things work better, but would take some advice on this.

This is a first go at fixing this issue. Most of this was taken from your suggestion in #48 and [this cool blog post](https://nathanleclaire.com/blog/2014/08/24/handling-ctrl-c-interrupt-signal-in-golang-programs/) It only handles SIGCONT, which is the signal sent to the application when you bring it back to the foreground. I don't think we need to worry about the initial SIGINT, which is when you press CTRL+Z. It works like this: 1. *signal.Notify* captures *syscall.SIGCONT* to channel c 1. function `handleSIGCONT` takes this channel as an argument, and is started as a goroutine. It waits for the signal before doing anything. 1. When SIGCONT occurs, `handleSIGCONT` fixes the display, then starts another instance of `handleSIGCONT` before finishing. 1. This repeats each time SIGCONT is captured A few things: 1. This might fit in an `init()` function 1. Is the recursion appropriate? Is it readable? 1. I am not sure how to fix the display. I chose those functions and things work better, but would take some advice on this.

sloum was assigned by asdf

2019-10-10 07:44:48 +00:00

asdf self-assigned this 2019-10-10 07:44:49 +00:00

asdf added the

bug

label 2019-10-10 07:45:32 +00:00

sloum reviewed 2019-10-10 14:19:38 +00:00

sloum left a comment

I left a few comments re: channels. Other than that it looks good. The only other thing I can think of is that maybe there needs to be a call to redraw the screen within handleSIGCONT? That way it forces the screen to go back to normal. I'm on my way out to work and will look at this more later, but you may have access to bombadillo.Draw() from there. If so, that would be the thing to do right at the end, before the next goroutine.

I left a few comments re: channels. Other than that it looks good. The only other thing I can think of is that maybe there needs to be a call to redraw the screen within `handleSIGCONT`? That way it forces the screen to go back to normal. I'm on my way out to work and will look at this more later, but you may have access to `bombadillo.Draw()` from there. If so, that would be the thing to do right at the end, before the next goroutine.

main.go Outdated

						
				@ -147,0 +154,4 @@

					cui.Tput("rmam")  // turn off line wrapping

					cui.Tput("smcup") // use alternate screen

					cui.SetCharMode()

					go handleSIGCONT(c)

sloum commented

2019-10-10 14:17:59 +00:00

Can channels be reused like this? I have used goroutines a good amount but had not had occasion until now to use a channel. So long as this works it looks fine and makes sense to me (the logic is clean and I actually really like the recursion)

sloum commented

2019-10-10 18:34:36 +00:00

I'm not certain, but this may be able to be rewritten:

for _, _ := <-c {
    cui.Tput("rmam")  // turn off line wrapping
    cui.Tput("smcup") // use alternate screen
    cui.SetCharMode()
    bombadillo.Draw() // not sure if this var is available here
}

I'm not certain that this behavior works and I'm not sure if the goroutine gets cleaned up on program exit properly or not. both are things to think about. But if this works, it saves passing the channel to a new goroutine.

I'm not certain, but this may be able to be rewritten: ``` for _, _ := <-c { cui.Tput("rmam") // turn off line wrapping cui.Tput("smcup") // use alternate screen cui.SetCharMode() bombadillo.Draw() // not sure if this var is available here } ``` I'm not certain that this behavior works and I'm not sure if the goroutine gets cleaned up on program exit properly or not. both are things to think about. But if this works, it saves passing the channel to a new goroutine.

sloum commented

2019-10-10 18:37:39 +00:00

Sorry, it wont let me edit the above. This is better:

for {
    <-c
    cui.Tput("rmam")  // turn off line wrapping
    cui.Tput("smcup") // use alternate screen
    cui.SetCharMode()
    bombadillo.Draw() // not sure if this var is available here
}

Sorry, it wont let me edit the above. This is better: ``` for { <-c cui.Tput("rmam") // turn off line wrapping cui.Tput("smcup") // use alternate screen cui.SetCharMode() bombadillo.Draw() // not sure if this var is available here } ```

asdf commented

2019-10-10 21:46:32 +00:00

In terms of fixing the display, adding bombadillo.Draw() now makes it work perfectly.
Regarding recursive routines, reuse of channels - I don't know enough to say. It works as it is, as well as when using the loop you suggest, but your advice raises a lot of things to look in to.

In terms of fixing the display, adding `bombadillo.Draw()` now makes it work perfectly. Regarding recursive routines, reuse of channels - I don't know enough to say. It works as it is, as well as when using the loop you suggest, but your advice raises a lot of things to look in to.

sloum commented

2019-10-11 01:55:10 +00:00

Awesome that adding the call to draw works! I'm not sure whether the loop or recursion would be better. I feel like the loop is, but I'm not really sure. Have you tested that subsequent ctrl+z and fg calls work? I am hopeful that they just work and that this issue is solved! Thanks for researching this. I read a good bit about channels while at work today and even talked them over with a co-worker. I get the concept but the usage in practice never fails to confuse me, lol.

main.go Outdated

						
				@ -156,0 +169,4 @@

					// buffered channel to capture SIGCONT for handling

					c := make(chan os.Signal, 1)

					signal.Notify(c, syscall.SIGCONT)

					go handleSIGCONT(c)

sloum commented

2019-10-10 14:16:52 +00:00

Are channels always pointers? Or does this need to be passed as a pointer?

asdf commented

2019-10-10 21:56:18 +00:00

I don't know enough to say, but a quick search find this that says:

...there is no reason to pass a pointer to a chan, map, or slice, or function value, since those are all reference types which internally contain a pointer (the exception would be if you want the callee to change the reference type header)...

As above, I need to do more reading to understand this better.

I don't know enough to say, but a quick search find [this](https://stackoverflow.com/questions/24868859/different-ways-to-pass-channels-as-arguments-in-function-in-go-golang) that says: <blockquote>...there is no reason to pass a pointer to a chan, map, or slice, or function value, since those are all reference types which internally contain a pointer (the exception would be if you want the callee to change the reference type header)...</blockquote> As above, I need to do more reading to understand this better.

sloum commented

2019-10-11 01:52:00 +00:00

That seems to answer the question pretty well :) Good to know that a channel is already a reference type :) Thanks for finding that

asdf reviewed 2019-10-11 01:10:48 +00:00

main.go

asdf commented

2019-10-11 01:10:48 +00:00

Indefinite loop used instead of recursive function

asdf reviewed 2019-10-11 01:11:55 +00:00

main.go

asdf commented

2019-10-11 01:11:55 +00:00

Added screen draw - this was the final piece, the display is shown correctly.

asdf reviewed 2019-10-11 01:12:53 +00:00

main.go

asdf commented

2019-10-11 01:12:53 +00:00

This now occurs after initClient() is called to ensure bombadillo.Draw() will exist.

This now occurs after `initClient()` is called to ensure `bombadillo.Draw()` will exist.

sloum approved these changes 2019-10-11 02:00:23 +00:00

sloum left a comment

These changes look good to me. I approve this fix into develop. I'll wait for confirmation that you are happy with where it is at and dont want to make further revisions but then I'll merge (or you can just merge in if you like)

main.go

						
				@ -166,6 +181,11 @@ func main() {

						panic(err)

					}

					// watch for SIGCONT, send it to be handled

sloum commented

2019-10-11 01:59:35 +00:00

👍

:thumbsup:

asdf commented

2019-10-11 02:04:32 +00:00

I'm ok with how this works now, and can't find anything wrong from what I've been reading.

While testing, I noticed that the initial CTRL+Z (SIGINT) is breaking the console a bit. Normally a flashing cursor is shown in the terminal, but while Bombadillo is suspended, it is not shown. The same occurs when CTRL+C (SIGTERM) is used. These are not new issues and are observable on the development branch.

These new issues can be handled by small extensions to this new functionality. It's probably worth adding them in before merging, but let me know if this isn't a good approach.

I'm ok with how this works now, and can't find anything wrong from what I've been reading. While testing, I noticed that the initial CTRL+Z (SIGINT) is breaking the console a bit. Normally a flashing cursor is shown in the terminal, but while Bombadillo is suspended, it is not shown. The same occurs when CTRL+C (SIGTERM) is used. These are not new issues and are observable on the development branch. These new issues can be handled by small extensions to this new functionality. It's probably worth adding them in before merging, but let me know if this isn't a good approach.

sloum commented

2019-10-11 02:48:46 +00:00

I think this approach forks fine. There is a simple vt-100 escape code to turn the cursor back on. That would need to happen when catching the SIGINT or SIGTSTP or whatever else changes to terminal out of "Bombadillo mode". I think this is a good step here so far and can be merged in (which I'll do now). Lets keep it in mind that we may want to listen for an interrupt signal at some point.

sloum closed this pull request

2019-10-11 02:49:07 +00:00

Sign in to join this conversation.

No reviewers