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

Input is still read after program quits #24

Closed
curio77 opened this issue Oct 30, 2020 · 25 comments
Closed

Input is still read after program quits #24

curio77 opened this issue Oct 30, 2020 · 25 comments
Labels
bug Something isn't working

Comments

@curio77
Copy link

curio77 commented Oct 30, 2020

If multiple tea.Programs are run in succession, each of them appears to set up a system signal handler (for catching SIGINT etc.), but does not clean it up after the program has quit without being interrupted this way. If you run n programs in succession that each want to catch "ctrl+c", for the n-th such program, you'll need to press Ctrl-C (n-1-m) times, where m is the number of previous programs that were interrupted. Of course, the interrupt-catching functionality should work independently of what has happened before.

@meowgorithm
Copy link
Member

Thanks for flagging this. If I'm reading correctly, this is something you've verified, correct?

Internally Bubble Tea listens for SIGWINCH only. ^C is handled via putting the terminal into raw mode and reading stdin.

@curio77
Copy link
Author

curio77 commented Oct 31, 2020

I haven't taken a look at your implementation (yet). I was just assuming you are catching the things like Ctrl-C via signal.Notify() but now learned you're doing this via raw terminal mode.

Anyway, the effect I've tried to describe is that for every previous tea.Program that has run, you need to press Ctrl-C one additional time in order to finally get the corresponding message in the latest-running program. To reproduce, run a program (that terminates), then another one that just waits for a "ctrl+c" message and observe that you'll need to press that combo twice to register as expected. If you were to run two programs before that, you'd have to press it three times.

@mritd
Copy link

mritd commented Dec 27, 2020

I also encountered a problem in my program, any keystroke takes a second time to respond.

@meowgorithm
Copy link
Member

Good to know. I'll bump this one up in priority.

Could you clarify what's happening with the keystrokes, exactly?

@mritd
Copy link

mritd commented Dec 27, 2020

When the TUI program is run twice in a row, the second time it runs, the key is pressed twice to respond;

This is my test code:

/~https://github.com/mritd/bubbles/blob/master/example/prompt/main.go#L12

When m1 is running, there will be no response to the first key press (the Update method is not called), and normal operation is restored after the second key press.

Note: After the "Please input password:" prompt, I pressed the "a" key twice, but the terminal only received it once.

2020-12-27 13 13 45

@meowgorithm
Copy link
Member

Thanks for that. Can you also post the source code for your prompt program?

@mritd
Copy link

mritd commented Dec 27, 2020

The source code is in the mritd/bubbles repository.

@meowgorithm
Copy link
Member

Wonderful, thank you. At first glance the prompt library looks fine (love the initData idea). I'll keep you updated as I look into this ticket.

@mritd
Copy link

mritd commented Dec 27, 2020

I just started learning bubbletea, bubbletea made me learn Elm Architecture, I am exploring some feasibility by writing a lot of code; sometimes I am not sure whether I am right, but I prefer to try. 😁😁😁

@mritd
Copy link

mritd commented Jan 11, 2021

@meowgorithm

I have found the problem and fixed it:

In fact, this is a goroutine leakage problem caused by io read blocking. I found a solution after reading a lot of articles.

Description of the cause of the problem

I tried many ways to solve this problem (I have some stupid ideas):

  • Add a "context" variable. After the blocked goroutine is successfully read, if it finds that the context has been cancelled, rewrite the read content to stdin (my stupid idea)
  • Attempt to "reopen" stdin when exiting (I did not try successfully)
  • Set SetReadDeadline for os.stdin (not supported)
  • Package os.stdin and find a way to add context to it
  • Some other unrealistic ideas 😂...

The final fix

Change the message channel into a global variable (the simplest fix, of course, I'm not sure if it is elegant and reasonable enough) /~https://github.com/charmbracelet/bubbletea/blob/v0.12.2/tea.go#L109

Some related articles:

mritd added a commit to mritd/bubbletea that referenced this issue Jan 11, 2021
Fix first input loss when running multiple times

fix charmbracelet#24

Signed-off-by: mritd <mritd@linux.com>
mritd added a commit to mritd/bubbles that referenced this issue Jan 11, 2021
fix bubbletea

see also charmbracelet/bubbletea#24

Signed-off-by: mritd <mritd@linux.com>
@muesli
Copy link
Contributor

muesli commented Jan 11, 2021

Nice find! I can see why your fix works, but I wonder if we couldn't close the channel and/or cancel the blocking subroutines, instead.

@mritd
Copy link

mritd commented Jan 11, 2021

@muesli Wait a minute, I am trying to add context to fix this problem.

mritd added a commit to mritd/bubbletea that referenced this issue Jan 11, 2021
add context to prevent goroutine leakage

see also charmbracelet#24

Signed-off-by: mritd <mritd@linux.com>
@mritd
Copy link

mritd commented Jan 11, 2021

@muesli I tried to add a context to prevent goroutine from leaking.
The code seems ugly. Does anyone have a better solution?

@meowgorithm
Copy link
Member

This is awesome, @mritd. Thanks for both the PR and all the details on the solution.

Since this is slightly behind master, I'm going to merge another PR and then bring your branch up to date. I have some thoughts about how to clean this up a little bit, too.

@meowgorithm meowgorithm changed the title System signal handlers stack if multiple programs are run in succession Input is still read after program quits Jan 12, 2021
meowgorithm pushed a commit that referenced this issue Jan 12, 2021
Fix first input loss when running multiple times

fix #24

Signed-off-by: mritd <mritd@linux.com>
meowgorithm pushed a commit that referenced this issue Jan 12, 2021
add context to prevent goroutine leakage

see also #24

Signed-off-by: mritd <mritd@linux.com>
@meowgorithm
Copy link
Member

meowgorithm commented Jan 12, 2021

So it would be ideal not to have the msgs channel as a global variable. The tricky thing is that, afaik, there’s no way to cancel a read from stdin. If we were to close the channel Go would panic if we're waiting on a read, which will almost always be the case.

@mritd
Copy link

mritd commented Jan 12, 2021

This is true, go does not allow cancellation of blocking read IO. This means that after the TUI program has finished running, the input of stdin will always be captured by another goroutine;

I tried to close stdin, but I cannot reopen it....

Perhaps it can be solved by some hacking methods, such as finding a way to kill this goroutine, but this is not an elegant solution; after reading some articles, most of the methods are to repackage this blocking IO.

@meowgorithm I don't have a better idea yet, but the discussion in this issue of the go official may inspire you:: golang/go#20280 😊

@meowgorithm
Copy link
Member

Okay, well good to know it's not just us who wants to cancel a Read. A couple more notes:

  • We could open a /dev/tty for input and then close it when done (mixed feelings about this)
  • Xiaq, the author of Elvish shell has implemented a cancellable read with deadlines (details)

As a side note, I'd personally implement the use cases I've seen in this issue as a single Bubble Tea program rather than multiple ones. For example, to prompt the user for a username and password I'd build those prompts as different states of one application. That would also allow your users to navigate back to a previous field to make corrections before submitting, too.

example.mov

Source code

@meowgorithm
Copy link
Member

So we just merged a PR that automatically opens a TTY for input if input's not TTY. In other words, if you pipe something into Bubble Tea it'll receive keypresses and so on as normal. If we either make that the default behavior or expose that behavior as something you opt into that should solve this one.

@nsaccente
Copy link

I pulled the code from the tutorials section of the bubbletea repo and made it a modular function call for use in my terminal programs. See the code in the details below, but I run the bubbletea shopping list program, and after the user quits, I read from stdin -- where the first input is ignored.

package main

import (
	"bufio"
	"fmt"
	"log"
	"os"
	"strings"

	tea "github.com/charmbracelet/bubbletea"
)

type listSelectorModel struct {
	prompt   string           // string prompt that tells the user what they're doing
	choices  []string         // items on the to-do list
	cursor   int              // which to-do list item our cursor is pointing at
	selected map[int]struct{} // which to-do items are selected
}

func main() {
	ListSelector("Favorite Pokemon: \n", []string{"Cindaquil", "Todadile", "Chickorita"}).Execute()
	x := Prompt("Enter something: ")
	fmt.Printf("You entered: %s", x)
}

// Prompt is a simple prompt which returns stripped keyboard input as a string.
func Prompt(prompt string) string {
	reader := bufio.NewReader(os.Stdin)
	fmt.Print(prompt)
	text, _ := reader.ReadString('\n')
	return strings.TrimSpace(text)
}

// ListSelector serves as a constructor for a listSelectorModel.
func ListSelector(prompt string, choices []string) listSelectorModel {
	return listSelectorModel{
		prompt:   prompt,
		choices:  choices,
		selected: make(map[int]struct{}),
	}
}

// Run the listSelectorModel and return an updated instance of the model.
func (initialModel listSelectorModel) Execute() []interface{} {
	tty, err := os.Open("/dev/tty")
	if err != nil {
		fmt.Println("could not open tty:", err)
		os.Exit(1)
	}
	defer tty.Close()
	p := tea.NewProgram(initialModel, tea.WithInput(tty))

	// Run the ListSelector with the model.
	// p := tea.NewProgram(initialModel)
	if err := p.Start(); err != nil {
		log.Fatal(err.Error())
	}

	result := make([]interface{}, 0)
	for k := range initialModel.selected {
		result = append(result, initialModel.choices[k])
	}
	return result
}

// Init is used by bubbletea and should be treated as private.
func (m listSelectorModel) Init() tea.Cmd {
	return nil // Just return `nil`, which means "no I/O right now, please."
}

// Update is used by bubbletea and should be treated as private.
func (m listSelectorModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
	switch msg := msg.(type) {
	case tea.KeyMsg:
		// Cool, what was the actual key pressed?
		switch msg.String() {

		// These keys should exit the program.
		case "ctrl+c", "q":
			return m, tea.Quit

		// The "up" and "k" keys move the cursor up
		case "up", "k":
			if m.cursor > 0 {
				m.cursor--
			}

		// The "down" and "j" keys move the cursor down
		case "down", "j":
			if m.cursor < len(m.choices)-1 {
				m.cursor++
			}

		// The "enter" key and the spacebar (a literal space) toggle
		// the selected state for the item that the cursor is pointing at.
		case "enter", " ":
			_, ok := m.selected[m.cursor]
			if ok {
				delete(m.selected, m.cursor)
			} else {
				m.selected[m.cursor] = struct{}{}
			}
		}
	}

	// Return the updated model to the Bubble Tea runtime for processing.
	// Note that we're not returning a command.
	return m, nil
}

// View is used by bubbletea and should be treated as private.
func (m listSelectorModel) View() string {
	s := m.prompt
	for i, choice := range m.choices {
		cursor := " "
		if m.cursor == i {
			cursor = ">"
		}
		checked := " "
		if _, ok := m.selected[i]; ok {
			checked = "x"
		}
		s += fmt.Sprintf("%s [%s] %s\n", cursor, checked, choice)
	}
	s += "\n(q to quit)\n"
	return s // Send the UI for rendering
}

Am I doing something wrong? Or is it possible this leaky channel is still affecting me?

@meowgorithm
Copy link
Member

@strickolas Ick. I'm able to reproduce that though I'm not sure the cause yet.

For the record, here's a stripped-down version that reproduces the issue.

package main

import (
	"bufio"
	"fmt"
	"os"
	"strings"

	tea "github.com/charmbracelet/bubbletea"
)

func main() {
	tty, err := os.Open("/dev/tty")
	if err != nil {
		fmt.Println("Could not open TTY:", err)
		os.Exit(1)
	}

	if err := tea.NewProgram(model{}, tea.WithInput(tty)).Start(); err != nil {
		fmt.Println("Error running program:", err)
		os.Exit(1)
	}

	tty.Close()

	reader := bufio.NewReader(os.Stdin)
	fmt.Print("Now type something: ")
	text, err := reader.ReadString('\n')
	if err != nil {
		fmt.Println("Error reading input:", err)
		os.Exit(1)
	}
	fmt.Printf("You entered: %s\n", strings.TrimSpace(text))
}

type model struct{}

func (m model) Init() tea.Cmd {
	return nil
}

func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
	if _, ok := msg.(tea.KeyMsg); ok {
		return m, tea.Quit
	}

	return m, nil
}

func (m model) View() string {
	return "Bubble Tea running. Press any key to exit...\n"
}

@nsaccente
Copy link

Was there ever a fix for this bug? I think MR #41 solves this -- but I'm not quite sure.

@meowgorithm
Copy link
Member

meowgorithm commented Jun 9, 2021

Unfortunately, not yet. #41 may solve the problem, but the implementation is less than ideal, so we've left the PR open. As mentioned earlier, this comes down to canceling a read, which is tricky in Go. This is not a case of a leaky channel.

You're welcome to have a look at this and submit a PR if you'd like. I've been meaning to look into applying the the implementation in Elvish, but have yet to do so.

@erikgeiser
Copy link
Contributor

I've got a rough POC that seems to work based on the select syscall. However, I had to comment out (*Program).shutdown(false) as this call seems to close os.Stdin but I don't know why. Afaik it should only affect the output.

@meowgorithm
Copy link
Member

This is now fixed in master and will be available in the next release. This bugfix was a huge undertaking by @erikgeiser who implemented a cancelable input reader with specific implementations for Linux, Windows and BSD, as well as a general implementation for all other unix systems.

@meowgorithm
Copy link
Member

This is now available in v0.16.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants