-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
Screen.Fini does not leave the terminal in a usable state in MacOS #394
Comments
I don’t think that was intentional behavior. Does it matter what the terminal emulator is? (E.g. if you come in via ssh in a Linux or Windows terminal? |
So, screen.Fini() is not meant for this kind of use -- it's designed to tear down on application exit, and unfortunately it is closing the stdin and stdout descriptors. Probably we need a better way to suspend the screen without closing it entirely. |
You could probably work around the problem by duping os.Stdin and os.Stdout. |
@gdamore Thanks for looking into this. Unfortunately, removing the call to Fini() does not fix the problem. The sample program @gokcehan posted still crashes the terminal: Terminal.app, iTerm2, or a Tmux pane. This seems to happen only on MacOS, not Linux. Similarly, using only duplicates of the filehandles (made before calling Tcell) does not help: func dup(fd int) int {
d, err := syscall.Dup(fd)
if err != nil {
panic(err)
}
return d
} cmd.Stdin = os.NewFile(uintptr(dup(syscall.Stdin)), "/dev/stdin")
cmd.Stdout = os.NewFile(uintptr(dup(syscall.Stdout)), "/dev/stdout")
cmd.Stderr = os.NewFile(uintptr(dup(syscall.Stderr)), "/dev/stderr") Here is a slightly simplified example that shows the problem: package main
import (
"fmt"
"log"
"os"
"os/exec"
"github.com/gdamore/tcell"
)
func clear() {
screen, err := tcell.NewScreen()
if err != nil {
log.Fatal("creating screen: %s", err)
}
if err := screen.Init(); err != nil {
log.Fatal("initializing screen: %s", err)
}
//defer screen.Fini()
screen.Clear()
}
func run(script string) error {
cmd := exec.Command("sh", "-c", script, "--")
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
return cmd.Run()
}
func main() {
f, err := os.Create("/tmp/lf-408.log")
if err != nil {
panic(err)
}
defer f.Close()
clear() // Removing this line avoids the crash.
fmt.Fprintln(f, "BEFORE") // always reached
if err := run("$SHELL"); err != nil {
fmt.Fprintf(f, "error: %#v\n", err) // never reached
panic(err)
}
fmt.Fprintln(f, "AFTER") // reached iff clear() is removed
} |
screen.Fini() needs to be called for sure, because your terminal will not be in cooked mode ... so you won't see output, etc. You're saying "crash", but the problems I saw seemed to be complaints that the started process was not connected to the terminal. Typically that's /dev/tty, but it could be different on macOS. Unfortunately I don't have a system handy to test with. |
"Crash" in the sense that the original example causes the parent terminal window (or Tmux pane or script(1) session) to close abruptly. The terminal is indeed /dev/tty on macOS. What is the best way to interleave Tcell calls with ordinary reads and writes of std{in,out,err}? In particular, the file manager @gokcehan mentioned makes good use of Tcell, but wants to temporarily hand off control of the terminal to an interactive subshell. Is this a supported use case? Interacting with Tcell, then attempting to spawn a subshell, seems to cause issues that vary by platform, including a terminal crash on MacOS. Any guidance is appreciated. For me at least, this isn't an urgent issue. |
So its not well supported but it probably should be. Its definitely not a "terminal crash". It is a problem where apparently the child process doesn't get a functioning tty. I'm not sure why that is, but I guess it's something different in macOS. |
Gotcha. What would the appropriate term be? Without tmux, this actually closes the parent Terminal.app or iTerm window. I would certainly call that a "terminal crash," but I'm happy to learn better terminology. |
I've been thinking about this, and I think I may have done pretty much everyone a disservice by opening /dev/tty directly. I think this would work better if I just used stdin and stdout. However that change is likely to break some applications so I need to think on it. |
@gdamore This is on the back of my mind but I didn't have a chance to look at it again. I still don't know an easy way to access to a Darwin VM and also my understanding of terminals are pretty limited. Still, I was thinking, could this be due to On a side note, what kind of changes should we expect from using stdin and stdout instead of tty? |
I do have a darwin system I can use if need be. Not closing the tty leads to other concerns, duplicate file descriptors and I/O that is misdirected. The main problem with using stdin/stdout is that if you want to redirect the application's output, then that won't work. There aren't many applications that want to use the GUI and do redirection, but some could theoretically exist. Note that e.g. vim doesn't let you do that -- it complains if stdin is not a tty and refuses to run. That would be the "new" semantic. |
Do you mean when it is reopened with
|
@jeffs , everyone: for reference, when you open This stackoverflow answer helps to clarify with detail: https://unix.stackexchange.com/questions/60641/linux-difference-between-dev-console-dev-tty-and-dev-tty0/384792#384792 (Side note, I don't have much to contribute to this issue at the moment though I am very interested in it's results of the discussion.) |
Yeah, the confusion around tty, ptys, console, and serial ttys is really unfortunate. I'm pretty sure that I'm going to simply stop accessing /dev/tty altogether, and hope that I can use stdin (and stdout) instead. My instinct is that I should also at that time stop explicitly opening or closing them. I'm not sure if golang has a nice check equivalent to isatty()... that would be helpful as well. |
@gdamore Getting rid of By the way, I don't know if you are aware, but Go team finally started moving ssh/terminal to it's own repository here: https://github.com/golang/term There is indeed an |
@gokcehan, @gdamore I literally just implemented https://github.com/pkg/term in my private fork of The sources have just been published at: https://github.com/kckrinke/go-cdk Highlights:
// Please note, my fork project is not intended to replace |
Interesting stuff all around. I'll have to have a look at the new x/term package... that might really help things out. |
@gdamore So with the recent change, the original example does not work on linux either. This is not necessarily a bad thing as I'm now able to work on this on my machine and hopefully a possible fix would be portable this time. The current issue seems to be that the input loop keeps consuming the input even after
I couldn't find an easy solution for this. Do you have any plans to make this work? You mention separate pause and resume functions for the api in the commit message but maybe that is not necessary. |
We have a terminal file manager in which we allow shell commands to be run with the ui paused temporarily, similar to
:!
commands in vim. We implemented this by callingScreen.Fini
before the shell command and then callingNewScreen
andScreen.Init
after the shell command. This seems to work on Windows and Linux but we get reports that it fails on MacOS. We have narrowed it down to the following toy example, which works in Linux but not MacOS.Is this a bug or intended behavior?
More information on our side and some altered examples in gokcehan/lf#480
cc @jeffs @thezeroalpha
The text was updated successfully, but these errors were encountered: