search

manifoldco / promptui

Interactive prompt for command-line applications
https://www.manifold.co
BSD 3-Clause "New" or "Revised" License
6.03k stars 333 forks source link

Data race between occuring on Cursor input #148

Open electrofelix opened 4 years ago

electrofelix commented 4 years ago

Following test example run with CGO_ENABLED=1 go test $(go list ./... | grep -v generated) -race shows the is an issue with how the cursor accesses it's input value

package promptui

import (
    "io/ioutil"
    "os"

    "testing"
)

// This example shows how to use the prompt for confirmation.
func TestPromptConfirm(t *testing.T) {

    t.Run("confirmed", func(t *testing.T) {
        tmpfile, err := ioutil.TempFile("", "confirm")
        if err != nil {
            t.Errorf("failed to create temporary file")
        }

        defer os.Remove(tmpfile.Name())

        tmpfile.WriteString("y\n")
        tmpfile.Seek(0, 0)

        prompt := Prompt{
            Label:     "Confirm action",
            IsConfirm: true,
            Stdin:     tmpfile,
        }

        _, err = prompt.Run()
        if err != nil {
            t.Errorf("failed to pick up confirmation")
        }
    })

    t.Run("not confirmed", func(t *testing.T) {
        tmpfile, err := ioutil.TempFile("", "confirm")
        if err != nil {
            t.Errorf("failed to create temporary file")
        }

        defer os.Remove(tmpfile.Name())

        tmpfile.WriteString("\n")
        tmpfile.Seek(0, 0)

        prompt := Prompt{
            Label:     "Confirm action",
            IsConfirm: true,
            Stdin:     tmpfile,
        }

        _, err = prompt.Run()
        if err == nil {
            t.Errorf("failed to pick up cancelled by user")
        }
    })
}

outputs lots of the following:

? Confirm action? [y/N] y█
==================
WARNING: DATA RACE
Read at 0x00c000012488 by goroutine 20:
  github.com/manifoldco/promptui.(*Cursor).Get()
      /home/electrofelix/git/github.com/manifoldco/promptui/cursor.go:144 +0x90e
  github.com/manifoldco/promptui.(*Prompt).Run()
      /home/electrofelix/git/github.com/manifoldco/promptui/prompt.go:193 +0x949
  github.com/manifoldco/promptui.TestPromptConfirm.func1()
      /home/electrofelix/git/github.com/manifoldco/promptui/example_confirm_test.go:30 +0x20b
  testing.tRunner()
      /home/electrofelix/local/gosdks/go1.14.1/src/testing/testing.go:992 +0x1eb

Previous write at 0x00c000012488 by goroutine 31:
  github.com/manifoldco/promptui.(*Cursor).Update()
      /home/electrofelix/git/github.com/manifoldco/promptui/cursor.go:138 +0x1572
  github.com/manifoldco/promptui.(*Cursor).Listen()
      /home/electrofelix/git/github.com/manifoldco/promptui/cursor.go:191 +0x12de
  github.com/manifoldco/promptui.(*Prompt).Run.func2()
      /home/electrofelix/git/github.com/manifoldco/promptui/prompt.go:159 +0xd6
  github.com/chzyer/readline.(*DumpListener).OnChange()
      /home/electrofelix/go/pkg/mod/github.com/chzyer/readline@v0.0.0-20180603132655-2972be24d48e/operation.go:516 +0x92
  github.com/chzyer/readline.(*Operation).ioloop()
      /home/electrofelix/go/pkg/mod/github.com/chzyer/readline@v0.0.0-20180603132655-2972be24d48e/operation.go:339 +0x759

Goroutine 20 (running) created at:
  testing.(*T).Run()
      /home/electrofelix/local/gosdks/go1.14.1/src/testing/testing.go:1043 +0x660
  github.com/manifoldco/promptui.TestPromptConfirm()
      /home/electrofelix/git/github.com/manifoldco/promptui/example_confirm_test.go:13 +0x5d
  testing.tRunner()
      /home/electrofelix/local/gosdks/go1.14.1/src/testing/testing.go:992 +0x1eb

Goroutine 31 (running) created at:
  github.com/chzyer/readline.NewOperation()
      /home/electrofelix/go/pkg/mod/github.com/chzyer/readline@v0.0.0-20180603132655-2972be24d48e/operation.go:88 +0x7f0
  github.com/chzyer/readline.(*Terminal).Readline()
      /home/electrofelix/go/pkg/mod/github.com/chzyer/readline@v0.0.0-20180603132655-2972be24d48e/terminal.go:95 +0x80
  github.com/chzyer/readline.NewEx()
      /home/electrofelix/go/pkg/mod/github.com/chzyer/readline@v0.0.0-20180603132655-2972be24d48e/readline.go:167 +0x5c
  github.com/manifoldco/promptui.(*Prompt).Run()
      /home/electrofelix/git/github.com/manifoldco/promptui/prompt.go:135 +0x344
  github.com/manifoldco/promptui.TestPromptConfirm.func1()
      /home/electrofelix/git/github.com/manifoldco/promptui/example_confirm_test.go:30 +0x20b
  testing.tRunner()
      /home/electrofelix/local/gosdks/go1.14.1/src/testing/testing.go:992 +0x1eb
==================
....
....

It appears that the update at https://github.com/manifoldco/promptui/blob/master/cursor.go#L138 and read at https://github.com/manifoldco/promptui/blob/master/cursor.go#L144 done by readline result in the input member to be accessed by two different goroutines triggering the race

electrofelix commented 4 years ago

The main reason this causes an issue is I was hoping to exercise the confirmation behaviour as a check in my code base without needing to mock out the promptui so that I could be sure a suitable error is returned and the code continued as expected if the required response was inputted by a user.

Unfortunately I hit a race similar to the above and AFAICT, there isn't any way to avoid it from the callers perspective.

Any thoughts on preferences for how best to resolve within the codebase here? Is a sync.Mutex Lock a good solution or should there be use of something like https://github.com/uber-go/atomic