charmbracelet / charmbracelet/bubbletea

bug: Kill() invoked shortly after Run() results in data races

Open
#1,689 1 comment 1 reaction 0 assignees View on GitHub
Dominant language
Go
Stars
44.9k
Forks
1.3k
PR merge metrics
No merged PRs in 30d

Description

**Describe the bug**
If you invoke `Kill`, shortly after invoking `Run`, it results in data races (shown below), in addition to some cases of `fatal error: sync: unlock of unlocked mutex` (couldn't reliably replicate). This could occur in a few scenarios, some examples:

- Short-duration tests, where a program is spun up and shut down very quickly (using `Kill` to ensure cleanup, where sending `Quit` doesn't necessarily guarantee this).
- `Program`'s spun up from potentially remote connections, where someone could theoretically introduce a lot of connections that terminate quickly.

**Setup**
Please complete the following information along with version numbers, if applicable.
- OS: n/a
- Shell: n/a
- Terminal Emulator: n/a
- Terminal Multiplexer: n/a

**To Reproduce**
The following test reproduces:

```go
func TestKillDuringStartupRace(t *testing.T) {
for range 100 {
p := NewProgram(&testModel{},
WithInput(bytes.NewBuffer(nil)),
WithOutput(io.Discard),
WithoutSignals(),
)

done := make(chan struct{})
go func() {
_, _ = p.Run()
close(done)
}()

p.Kill()

select {
case <-done:
case <-time.After(time.Second):
t.Fatal("program did not stop")
}
}
}
```

**Expected behavior**

Due to `Run` not really providing a way to know when things are "started enough", it's hard to know when or when not `Kill` should be safe to invoke. I would expect one of these two:

1. Provide a way to know when it's safe to invoke `Kill`.
2. Make `Kill` block until startup is safe enough to easily be cancelled/"unrolled". However, this still needs to ensure that it can't run into a scenario where it blocks forever.

**Additional context**

go test running with -race, showing the data race

```
$ go test -race -timeout 20s -count 20 -cpu 1,4 -run 'TestKillDuringStartupRace$' .
==================
WARNING: DATA RACE
Write at 0x00c0000b47b0 by goroutine 10:
charm.land/bubbletea/v2.(*Program).Run()
/home/liam/tmp/fork-bubbletea/tea.go:997 +0x87
charm.land/bubbletea/v2.TestKillDuringStartupRace.func1()
/home/liam/tmp/fork-bubbletea/tea_test.go:230 +0x33

Previous read at 0x00c0000b47b0 by goroutine 9:
charm.land/bubbletea/v2.(*channelHandlers).shutdown()
/home/liam/tmp/fork-bubbletea/tea.go:415 +0xd2
charm.land/bubbletea/v2.TestKillDuringStartupRace.(*Program).Kill.(*Program).shutdown.func5()
/home/liam/tmp/fork-bubbletea/tea.go:1246 +0x58
sync.(*Once).doSlow()
/usr/local/go/src/sync/once.go:78 +0xa1
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.(*T).Run.gowrap1()
/usr/local/go/src/testing/testing.go:2101 +0x38

Goroutine 10 (running) created at:
charm.land/bubbletea/v2.TestKillDuringStartupRace()
/home/liam/tmp/fork-bubbletea/tea_test.go:229 +0x359
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.(*T).Run.gowrap1()
/usr/local/go/src/testing/testing.go:2101 +0x38

Goroutine 9 (running) created at:
testing.(*T).Run()
/usr/local/go/src/testing/testing.go:2101 +0xb12
testing.runTests.func1()
/usr/local/go/src/testing/testing.go:2585 +0x85
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.runTests()
/usr/local/go/src/testing/testing.go:2583 +0x9e9
testing.(*M).Run()
/usr/local/go/src/testing/testing.go:2443 +0xf4b
main.main()
_testmain.go:102 +0x164
==================
==================
WARNING: DATA RACE
Write at 0x00c0000b4888 by goroutine 10:
charm.land/bubbletea/v2.(*Program).Run()
/home/liam/tmp/fork-bubbletea/tea.go:1077 +0xc24
charm.land/bubbletea/v2.TestKillDuringStartupRace.func1()
/home/liam/tmp/fork-bubbletea/tea_test.go:230 +0x33

Previous read at 0x00c0000b4888 by goroutine 9:
charm.land/bubbletea/v2.TestKillDuringStartupRace.(*Program).Kill.(*Program).shutdown.func5()
/home/liam/tmp/fork-bubbletea/tea.go:1259 +0xf1
sync.(*Once).doSlow()
/usr/local/go/src/sync/once.go:78 +0xa1
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.(*T).Run.gowrap1()
/usr/local/go/src/testing/testing.go:2101 +0x38

Goroutine 10 (running) created at:
charm.land/bubbletea/v2.TestKillDuringStartupRace()
/home/liam/tmp/fork-bubbletea/tea_test.go:229 +0x359
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.(*T).Run.gowrap1()
/usr/local/go/src/testing/testing.go:2101 +0x38

Goroutine 9 (running) created at:
testing.(*T).Run()
/usr/local/go/src/testing/testing.go:2101 +0xb12
testing.runTests.func1()
/usr/local/go/src/testing/testing.go:2585 +0x85
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.runTests()
/usr/local/go/src/testing/testing.go:2583 +0x9e9
testing.(*M).Run()
/usr/local/go/src/testing/testing.go:2443 +0xf4b
main.main()
_testmain.go:102 +0x164
==================
==================
WARNING: DATA RACE
Write at 0x00c0000b48e8 by goroutine 10:
charm.land/bubbletea/v2.(*Program).initInputReader()
/home/liam/tmp/fork-bubbletea/tty.go:69 +0x22c
charm.land/bubbletea/v2.(*Program).Run()
/home/liam/tmp/fork-bubbletea/tea.go:1101 +0x11a9
charm.land/bubbletea/v2.TestKillDuringStartupRace.func1()
/home/liam/tmp/fork-bubbletea/tea_test.go:230 +0x33

Previous read at 0x00c0000b48e8 by goroutine 9:
charm.land/bubbletea/v2.TestKillDuringStartupRace.(*Program).Kill.(*Program).shutdown.func5()
/home/liam/tmp/fork-bubbletea/tea.go:1249 +0x6d
sync.(*Once).doSlow()
/usr/local/go/src/sync/once.go:78 +0xa1
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.(*T).Run.gowrap1()
/usr/local/go/src/testing/testing.go:2101 +0x38

Goroutine 10 (running) created at:
charm.land/bubbletea/v2.TestKillDuringStartupRace()
/home/liam/tmp/fork-bubbletea/tea_test.go:229 +0x359
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.(*T).Run.gowrap1()
/usr/local/go/src/testing/testing.go:2101 +0x38

Goroutine 9 (running) created at:
testing.(*T).Run()
/usr/local/go/src/testing/testing.go:2101 +0xb12
testing.runTests.func1()
/usr/local/go/src/testing/testing.go:2585 +0x85
testing.tRunner()
/usr/local/go/src/testing/testing.go:2036 +0x21c
testing.runTests()
/usr/local/go/src/testing/testing.go:2583 +0x9e9
testing.(*M).Run()
/usr/local/go/src/testing/testing.go:2443 +0xf4b
main.main()
_testmain.go:102 +0x164
==================
--- FAIL: TestKillDuringStartupRace (0.08s)
testing.go:1712: race detected during execution of test
FAIL
FAIL charm.land/bubbletea/v2 12.195s
FAIL
```

Contributor guide

Open the contributing guide

Research direction

Start with the KillDuringStartupRace reproduction in tea_test.go and run it under the race detector with the command shown. Trace the startup and shutdown paths in tea.go, especially Run and Kill, then inspect channelHandlers.shutdown in tea.go and initInputReader in tty.go. Done means the reproduction completes without data races, mutex panics, or failure to stop.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.