Screen.Fini does not leave the terminal in a usable state in MacOS

[gokcehan]

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 calling Screen.Fini before the shell command and then calling NewScreen and Screen.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.

package main

import (
	"fmt"
	"log"
	"os"
	"os/exec"

	"github.com/gdamore/tcell"
)

func emit(screen tcell.Screen, x, y int, msg string) {
	for _, c := range msg {
		screen.SetContent(x, y, c, nil, tcell.StyleDefault)
		x++
	}
}

func display(msg string) {
	var screen tcell.Screen
	var err error

	if screen, err = tcell.NewScreen(); err != nil {
		log.Fatal("creating screen: %s", err)
	} else if err = screen.Init(); err != nil {
		log.Fatal("initializing screen: %s", err)
	}

	screen.Clear()
	emit(screen, 1, 1, msg)
	screen.Show()

loop:
	for {
		switch screen.PollEvent().(type) {
		case *tcell.EventKey:
			break loop
		}
	}

	screen.Fini()
}

func main() {
	cmd := exec.Command("sh", "-c", "$SHELL", "--")

	cmd.Stdin = os.Stdin
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr

	display("press any key to spawn a shell")

	fmt.Println("spawning a shell... (try a few commands and then 'exit')")

	if err := cmd.Run(); err != nil {
		panic(err)
	}

	fmt.Println("shell is properly finished.")

	display("press any key to quit")
}

Is this a bug or intended behavior?

More information on our side and some altered examples in gokcehan/lf#480

cc @jeffs @thezeroalpha

[gdamore]

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?

[gdamore]

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.

[gdamore]

You could probably work around the problem by duping os.Stdin and os.Stdout.

[jeffs]

@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
}

[gdamore]

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.

[jeffs]

"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.

Here's a video of what I'm seeing, with and without Fini():

[gdamore]

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.

[jeffs]

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.

[gdamore]

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.

[gokcehan]

@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 termioFini closing tScreen.in and tScreen.out? This seems to work without an issue on Linux but maybe there is some kind of poor interaction for adopting a tty file closed by a parent process on Darwin? Do you think it might work simply to remove those close statements? If Fini is meant to be called on program exit, then there shouldn't be any need to close the files anyways. Even if not, then GC should be able to close open files without a reference anyways, so I don't expect any leaks about it. Of course this is just a hunch as I wasn't able to test this on Darwin, but I feel like there should be a way to make this work since ncurses also seems to use /dev/tty.

On a side note, what kind of changes should we expect from using stdin and stdout instead of tty?

[gdamore]

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.

[gokcehan]

Not closing the tty leads to other concerns, duplicate file descriptors and I/O that is misdirected.

Do you mean when it is reopened with termioInit afterwards? How about moving nil check to the init functions then? Something along the line:

if t.in == nil {
	if t.in, e = os.OpenFile("/dev/tty", os.O_RDONLY, 0); e != nil {
		goto failed
	}
}
if t.out == nil {
	if t.out, e = os.OpenFile("/dev/tty", os.O_WRONLY, 0); e != nil {
		goto failed
	}
}

[kckrinke]

@jeffs , everyone: for reference, when you open /dev/tty, you're actually opening the terminal of the controlling process and not something along the lines of a pty or tty0. For tcell this is typically the terminal of the shell it's being run within, which means that if your application segfaults poorly, it may in fact take the parent process with it. This is the behavior you're seeing with respect to the disappearing terminal session.

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.)

[gdamore]

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.

[gokcehan]

@gdamore Getting rid of /dev/tty sounds like a good plan.

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 IsTerminal function to check tty. There are also other functions like MakeRaw/Restore and GetSize which you might delegate to make your job even simpler.

[kckrinke]

@gokcehan, @gdamore I literally just implemented https://github.com/pkg/term in my private fork of tcell. I've also just finished testing on Mac and the issue of corrupted state on exit has been resolved. I'm not seeing this issue anymore. Haven't tested on bsd, solaris or windows. Only Mac and Linux with Go v1.15.6.

The sources have just been published at: https://github.com/kckrinke/go-cdk

Highlights:

• termioInit now takes a string argument, if "" uses /dev/tty
• termioClose and termioInit are now essentially the same for all platforms
• see tscreen_darwin.go

// Please note, my fork project is not intended to replace tcell and in fact recommends to not use my project at all though this is an entirely different discussion, not relevant here.

[gdamore]

Interesting stuff all around.

I'll have to have a look at the new x/term package... that might really help things out.

[gokcehan]

@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 Fini is called. The following almost works but the first character is lost as Read is blocking.

diff --git a/tscreen.go b/tscreen.go
index c492d22..99420c2 100644
--- a/tscreen.go
+++ b/tscreen.go
@@ -1490,6 +1490,11 @@ func (t *tScreen) mainLoop() {
 func (t *tScreen) inputLoop() {

        for {
+               select {
+               case <-t.quit:
+                       return
+               default:
+               }
                chunk := make([]byte, 128)
                n, e := t.in.Read(chunk)
                switch e {

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.