From 93392fc149033cb49dfc39bbba7bc8427799ed6d Mon Sep 17 00:00:00 2001 From: Ernest Romero Climent Date: Tue, 10 May 2022 01:18:01 -0700 Subject: [PATCH] Fix terminal state and key state data races (#335) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update gitignore to ignore .swp * Fix terminal state data races go run -race cmd/darktile/main.go WARNING: DATA RACE Write at 0x00c000864128 by goroutine 23: github.com/liamg/darktile/internal/app/darktile/termutil.(*Terminal).sgrSequenceHandler() /home/ernestrc/src/darktile/internal/app/darktile/termutil/csi.go:973 +0x12ca github.com/liamg/darktile/internal/app/darktile/termutil.(*Terminal).handleCSI() /home/ernestrc/src/darktile/internal/app/darktile/termutil/csi.go:62 +0x7e8 github.com/liamg/darktile/internal/app/darktile/termutil.(*Terminal).handleANSI() /home/ernestrc/src/darktile/internal/app/darktile/termutil/ansi.go:11 +0xc86 github.com/liamg/darktile/internal/app/darktile/termutil.(*Terminal).process() /home/ernestrc/src/darktile/internal/app/darktile/termutil/terminal.go:206 +0x126 github.com/liamg/darktile/internal/app/darktile/termutil.(*Terminal).Run·dwrap·10() /home/ernestrc/src/darktile/internal/app/darktile/termutil/terminal.go:171 +0x39 Previous read at 0x00c000864128 by goroutine 22: [failed to restore the stack] Goroutine 23 (running) created at: github.com/liamg/darktile/internal/app/darktile/termutil.(*Terminal).Run() /home/ernestrc/src/darktile/internal/app/darktile/termutil/terminal.go:171 +0x517 github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).Run.func1() /home/ernestrc/src/darktile/internal/app/darktile/gui/gui.go:80 +0xa Goroutine 22 (running) created at: github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw.(*UserInterface).Run() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw/run_notsinglethread.go:37 +0x2c4 github.com/hajimehoshi/ebiten/v2.RunGame() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/run.go:158 +0x1d4 github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).Run() /home/ernestrc/src/darktile/internal/app/darktile/gui/gui.go:99 +0x2de github.com/liamg/darktile/internal/app/darktile/cmd.glob..func2() /home/ernestrc/src/darktile/internal/app/darktile/cmd/root.go:130 +0x13a9 github.com/spf13/cobra.(*Command).execute() /home/ernestrc/src/darktile/vendor/github.com/spf13/cobra/command.go:852 +0xa7d github.com/spf13/cobra.(*Command).ExecuteC() /home/ernestrc/src/darktile/vendor/github.com/spf13/cobra/command.go:960 +0x5da github.com/spf13/cobra.(*Command).Execute() /home/ernestrc/src/darktile/vendor/github.com/spf13/cobra/command.go:897 +0x366 github.com/liamg/darktile/internal/app/darktile/cmd.Execute() /home/ernestrc/src/darktile/internal/app/darktile/cmd/root.go:153 +0x34f main.main() /home/ernestrc/src/darktile/cmd/darktile/main.go:75 +0x24 ================== Found 26 data race(s) * Fix KeyPressed data race go run -race cmd/darktile/main.go WARNING: DATA RACE Write at 0x00c00009fbc0 by goroutine 20: runtime.mapdelete_fast64() /usr/lib/go/src/runtime/map_fast64.go:272 +0x0 github.com/liamg/darktile/internal/app/darktile/gui.(*keyState).RepeatPressed() /home/ernestrc/src/darktile/internal/app/darktile/gui/key_states.go:61 +0x428 github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).handleInput() /home/ernestrc/src/darktile/internal/app/darktile/gui/input.go:109 +0x224 github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).Update() /home/ernestrc/src/darktile/internal/app/darktile/gui/update.go:34 +0x2e github.com/hajimehoshi/ebiten/v2.(*imageDumper).update() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/imagedumper_desktop.go:111 +0x85 github.com/hajimehoshi/ebiten/v2.(*imageDumperGame).Update() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/run.go:115 +0x172 github.com/hajimehoshi/ebiten/v2.(*uiContext).updateImpl() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/uicontext.go:194 +0x12c github.com/hajimehoshi/ebiten/v2.(*uiContext).update() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/uicontext.go:166 +0x88 github.com/hajimehoshi/ebiten/v2.(*uiContext).Update() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/uicontext.go:147 +0x3b github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw.(*UserInterface).loop() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw/ui.go:1036 +0x401 github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw.(*UserInterface).Run.func1() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw/run_notsinglethread.go:53 +0x1b1 Previous read at 0x00c00009fbc0 by goroutine 19: github.com/liamg/darktile/internal/app/darktile/gui.(*keyState).AnythingPressed() /home/ernestrc/src/darktile/internal/app/darktile/gui/key_states.go:33 +0xbc github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).watchForUpdate() /home/ernestrc/src/darktile/internal/app/darktile/gui/gui.go:105 +0x77 github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).Run·dwrap·2() /home/ernestrc/src/darktile/internal/app/darktile/gui/gui.go:97 +0x39 Goroutine 20 (running) created at: github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw.(*UserInterface).Run() /home/ernestrc/src/darktile/vendor/github.com/hajimehoshi/ebiten/v2/internal/uidriver/glfw/run_notsinglethread.go:37 +0x2c4 github.com/hajimehoshi/ebiten/v2.RunGame() Goroutine 19 (running) created at: github.com/liamg/darktile/internal/app/darktile/gui.(*GUI).Run() /home/ernestrc/src/darktile/internal/app/darktile/gui/gui.go:97 +0x2ca github.com/liamg/darktile/internal/app/darktile/cmd.glob..func2() /home/ernestrc/src/darktile/internal/app/darktile/cmd/root.go:130 +0x13a9 github.com/spf13/cobra.(*Command).execute() /home/ernestrc/src/darktile/vendor/github.com/spf13/cobra/command.go:852 +0xa7d github.com/spf13/cobra.(*Command).ExecuteC() /home/ernestrc/src/darktile/vendor/github.com/spf13/cobra/command.go:960 +0x5da github.com/spf13/cobra.(*Command).Execute() /home/ernestrc/src/darktile/vendor/github.com/spf13/cobra/command.go:897 +0x366 github.com/liamg/darktile/internal/app/darktile/cmd.Execute() /home/ernestrc/src/darktile/internal/app/darktile/cmd/root.go:153 +0x34f main.main() /home/ernestrc/src/darktile/cmd/darktile/main.go:75 +0x24 --- .gitignore | 1 + internal/app/darktile/gui/gui.go | 8 ++-- internal/app/darktile/gui/input.go | 2 + internal/app/darktile/gui/key_states.go | 2 + internal/app/darktile/gui/render/render.go | 2 + internal/app/darktile/gui/resize.go | 7 +++- internal/app/darktile/termutil/ansi.go | 3 ++ internal/app/darktile/termutil/terminal.go | 43 +++++++++++++++------- 8 files changed, 48 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index cf7a9a7..2664ab5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ .idea .vscode /darktile +*.swp diff --git a/internal/app/darktile/gui/gui.go b/internal/app/darktile/gui/gui.go index a2614c0..e501238 100644 --- a/internal/app/darktile/gui/gui.go +++ b/internal/app/darktile/gui/gui.go @@ -102,12 +102,12 @@ func (g *GUI) Run() error { func (g *GUI) watchForUpdate() { for range g.updateChan { ebiten.ScheduleFrame() - go func() { - if g.keyState.AnythingPressed() { + if g.keyState.AnythingPressed() { + go func() { time.Sleep(time.Millisecond * 10) ebiten.ScheduleFrame() - } - }() + }() + } } } diff --git a/internal/app/darktile/gui/input.go b/internal/app/darktile/gui/input.go index 7122c5a..a974a88 100644 --- a/internal/app/darktile/gui/input.go +++ b/internal/app/darktile/gui/input.go @@ -37,6 +37,8 @@ var modifiableKeys = map[ebiten.Key]uint8{ } func (g *GUI) handleInput() error { + g.terminal.Lock() + defer g.terminal.Unlock() if err := g.handleMouse(); err != nil { return err diff --git a/internal/app/darktile/gui/key_states.go b/internal/app/darktile/gui/key_states.go index d650381..6ea4163 100644 --- a/internal/app/darktile/gui/key_states.go +++ b/internal/app/darktile/gui/key_states.go @@ -30,6 +30,8 @@ type press struct { } func (k *keyState) AnythingPressed() bool { + k.mu.Lock() + defer k.mu.Unlock() return len(k.keys) > 0 } diff --git a/internal/app/darktile/gui/render/render.go b/internal/app/darktile/gui/render/render.go index 359e069..5a523e8 100644 --- a/internal/app/darktile/gui/render/render.go +++ b/internal/app/darktile/gui/render/render.go @@ -62,6 +62,8 @@ func New(screen *ebiten.Image, terminal *termutil.Terminal, fontManager *font.Ma } func (r *Render) Draw() { + r.terminal.Lock() + defer r.terminal.Unlock() // 1. fill frame with default background colour r.frame.Fill(r.theme.DefaultBackground()) diff --git a/internal/app/darktile/gui/resize.go b/internal/app/darktile/gui/resize.go index 9c5b830..1611f17 100644 --- a/internal/app/darktile/gui/resize.go +++ b/internal/app/darktile/gui/resize.go @@ -22,14 +22,17 @@ func (g *GUI) Layout(outsideWidth, outsideHeight int) (int, int) { func (g *GUI) resize(w, h int) { - if g.fontManager.CharSize().X == 0 || g.fontManager.CharSize().Y == 0 { + if g.fontManager.CharSize().X == 0 || g.fontManager.CharSize().Y == 0 || g.terminal == nil { return } cols := uint16(w / g.fontManager.CharSize().X) rows := uint16(h / g.fontManager.CharSize().Y) - if g.terminal != nil && g.terminal.IsRunning() { + g.terminal.Lock() + defer g.terminal.Unlock() + + if g.terminal.IsRunning() { _ = g.terminal.SetSize(rows, cols) } } diff --git a/internal/app/darktile/termutil/ansi.go b/internal/app/darktile/termutil/ansi.go index e7d67e8..421e4dd 100644 --- a/internal/app/darktile/termutil/ansi.go +++ b/internal/app/darktile/termutil/ansi.go @@ -6,6 +6,9 @@ func (t *Terminal) handleANSI(readChan chan MeasuredRune) (renderRequired bool) t.log("ANSI SEQ %c 0x%X", r.Rune, r.Rune) + t.mu.Lock() + defer t.mu.Unlock() + switch r.Rune { case '[': return t.handleCSI(readChan) diff --git a/internal/app/darktile/termutil/terminal.go b/internal/app/darktile/termutil/terminal.go index b1eb669..9d9ca1c 100644 --- a/internal/app/darktile/termutil/terminal.go +++ b/internal/app/darktile/termutil/terminal.go @@ -7,6 +7,7 @@ import ( "io" "os" "os/exec" + "sync" "github.com/creack/pty" "golang.org/x/term" @@ -20,6 +21,7 @@ const ( // Terminal communicates with the underlying terminal type Terminal struct { + mu sync.Mutex windowManipulator WindowManipulator pty *os.File updateChan chan struct{} @@ -196,17 +198,20 @@ func (t *Terminal) requestRender() { } } +func (t *Terminal) processSequence(mr MeasuredRune) (render bool) { + if mr.Rune == 0x1b { + return t.handleANSI(t.processChan) + } + return t.processRunes(mr) +} + func (t *Terminal) process() { for { select { case <-t.closeChan: return case mr := <-t.processChan: - if mr.Rune == 0x1b { // ANSI escape char, which means this is a sequence - if t.handleANSI(t.processChan) { - t.requestRender() - } - } else if t.processRunes(mr) { // otherwise it's just an individual rune we need to process + if t.processSequence(mr) { t.requestRender() } } @@ -214,6 +219,8 @@ func (t *Terminal) process() { } func (t *Terminal) processRunes(runes ...MeasuredRune) (renderRequired bool) { + t.mu.Lock() + defer t.mu.Unlock() for _, r := range runes { @@ -226,31 +233,31 @@ func (t *Terminal) processRunes(runes ...MeasuredRune) (renderRequired bool) { //DING DING DING continue case 0x8: //backspace - t.GetActiveBuffer().backspace() + t.activeBuffer.backspace() renderRequired = true case 0x9: //tab - t.GetActiveBuffer().tab() + t.activeBuffer.tab() renderRequired = true case 0xa, 0xc: //newLine/form feed - t.GetActiveBuffer().newLine() + t.activeBuffer.newLine() renderRequired = true case 0xb: //vertical tab - t.GetActiveBuffer().verticalTab() + t.activeBuffer.verticalTab() renderRequired = true case 0xd: //carriageReturn - t.GetActiveBuffer().carriageReturn() + t.activeBuffer.carriageReturn() renderRequired = true case 0xe: //shiftOut - t.GetActiveBuffer().currentCharset = 1 + t.activeBuffer.currentCharset = 1 case 0xf: //shiftIn - t.GetActiveBuffer().currentCharset = 0 + t.activeBuffer.currentCharset = 0 default: if r.Rune < 0x20 { // handle any other control chars here? continue } - t.GetActiveBuffer().write(t.translateRune(r)) + t.activeBuffer.write(t.translateRune(r)) renderRequired = true } } @@ -259,7 +266,7 @@ func (t *Terminal) processRunes(runes ...MeasuredRune) (renderRequired bool) { } func (t *Terminal) translateRune(b MeasuredRune) MeasuredRune { - table := t.GetActiveBuffer().charsets[t.GetActiveBuffer().currentCharset] + table := t.activeBuffer.charsets[t.activeBuffer.currentCharset] if table == nil { return b } @@ -306,3 +313,11 @@ func (t *Terminal) useMainBuffer() { func (t *Terminal) useAltBuffer() { t.switchBuffer(AltBuffer) } + +func (t *Terminal) Lock() { + t.mu.Lock() +} + +func (t *Terminal) Unlock() { + t.mu.Unlock() +}