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

terminal is left scrambled when the app exits #247

Closed
alessio opened this issue Oct 9, 2020 · 5 comments
Closed

terminal is left scrambled when the app exits #247

alessio opened this issue Oct 9, 2020 · 5 comments
Assignees
Labels
question Further information is requested

Comments

@alessio
Copy link

alessio commented Oct 9, 2020

Hi,

One of my colleagues has written a gorgeous terminal-based block explorer application using termdash: https://github.com/Tosch110/gex

While testing his application, I've noticed that when the app crashes, the terminal is left in bad state - pretty much as described in this upstream bug nsf/termbox-go#182 (comment).
Generally, I'd love to restore the terminal state so that users can continue working in the same terminal session after my app exits.

Any advice would be greatly appreciated.
Thanks for considering.

@mum4k
Copy link
Owner

mum4k commented Oct 13, 2020

Hi @alessio, thank you for sharing the link to gex with me, that looks awesome!

Thank you for reporting this, I agree that it would be ideal if the terminal is correctly returned into the CLI mode even after unexpected crashes. I have a request if I may. Before we deep dive into termbox-go and figure out how to recover the terminal during a panic - would you be willing to give tcell a try? tcell is aiming to be a "newer and better" terminal layer. Termdash can now work with either one of the two, so it should be trivial to test. Here is some documentation for the tcell integration:

https://github.com/mum4k/termdash/wiki/Tcell-API

And an example of use is in the termdashdemo:

case termboxTerminal:
t, err = termbox.New(termbox.ColorMode(terminalapi.ColorMode256))
case tcellTerminal:
t, err = tcell.New(tcell.ColorMode(terminalapi.ColorMode256))

There is a chance that the cleanup will just work with tcell, however even if it doesn't it might be a better time investment if we focus on fixing the interaction with tcell than investing into termbox-go.

Please let me know what you find out.

@mum4k mum4k added the question Further information is requested label Oct 13, 2020
@alessio
Copy link
Author

alessio commented Oct 15, 2020

Hey @mum4k - thanks for taking the time!

tcell does not seem to provide a better experience

@mum4k
Copy link
Owner

mum4k commented Oct 15, 2020

Thank you @alessio for giving tcell a try. Since both termbox-go and tcell have this problem, it is likely that there is some cleanup code we could be calling even on an application panic.

I will look into it and try to identify some improvement. Once we know how to perform a correct cleanup, we can use Go's recover functionality to catch a panic and recover the terminal before finally crashing.

@mum4k mum4k added bug Something isn't working and removed question Further information is requested labels Oct 15, 2020
@mum4k mum4k self-assigned this Oct 15, 2020
@mum4k
Copy link
Owner

mum4k commented Oct 19, 2020

@alessio, after looking at this a bit more and reminding myself of how panic handling works in Go, I don't think we have a good solution available. At least not in the infrastructure layer provided by termdash. The relevant part of the Go spec:

https://golang.org/ref/spec#Handling_panics

While executing a function F, an explicit call to panic or a run-time panic terminates the execution of F. Any functions deferred by F are then executed as usual. Next, any deferred functions run by F's caller are run, and so on up to any deferred by the top-level function in the executing goroutine. At that point, the program is terminated and the error condition is reported, including the value of the argument to panic. This termination sequence is called panicking.

To clean and reset the terminal correctly, we need to make sure the Close method on termbox or tcell gets executed. E.g. the demo defers that call here:

defer t.Close()

My assumption is that the panics you are dealing with originate in a different goroutine than the one that defers the call to the close method. Based on the definition in the spec - only deferred functions in the panicking goroutine get executed, then the program terminates. Since in Go other goroutines have no means of affecting or catching a panic in a separate goroutine, no code in termdash can change this behavior.

The best solution I can think of needs to be done in the code that uses termdash. You could include a deferred call to recover() in the main goroutine of your application (the one that could potentially panic). This recover would call the mentioned Close method. Note that the objects the Close method is defined on aren't thread safe, so the deferred function would have to terminate termdash correctly before calling Close. Otherwise the call to Close could result in more unspecified behavior. Correct termination of termdash can be done by canceling the context used by termdash which if thread safe from any goroutine:

Please let me know if this helps or if you think there is a way how we can fix this in the infrastructure layer.

@mum4k mum4k added question Further information is requested and removed bug Something isn't working labels Oct 19, 2020
@mum4k
Copy link
Owner

mum4k commented Nov 10, 2020

Going to close this for now, but please feel free to reopen if you have further questions or ideas how to solve this at the termdash layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants