Fix terminal state and key state data races (#335)

* 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
liamg · May 10, 2022 · 93392fc · 93392fc
1 parent 62453da
commit 93392fc
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,24 +198,29 @@ 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()
 			}
 		}
 	}
 }
 
 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()
+}