fix data races (#230) (#235)

* fix data races (#230)

* Update CODEOWNERS

.github/CODEOWNERS

@@ -1 +1 @@
-* @liamg
+* @liamg @MaxRis

gui/gui.go

@@ -173,15 +173,23 @@ func (gui *GUI) scale() float32 {
 }
 
 // can only be called on OS thread
-func (gui *GUI) resizeToTerminal(newCols uint, newRows uint) {
+func (gui *GUI) resizeToTerminal() {
 
 	if gui.window.GetAttrib(glfw.Iconified) != 0 {
 		return
 	}
 
+	// Order of locking:
+	// 1. resizeLock
+	// 2. terminal's lock
 	gui.resizeLock.Lock()
 	defer gui.resizeLock.Unlock()
+	gui.terminal.Lock()
+	defer gui.terminal.Unlock()
 
+	termCols, termRows := gui.terminal.GetSize()
+	newCols := uint(termCols)
+	newRows := uint(termRows)
 	cols, rows := gui.renderer.GetTermSize()
 	if cols == newCols && rows == newRows {
 		return
@@ -244,9 +252,16 @@ func (gui *GUI) resize(w *glfw.Window, width int, height int) {
 		return
 	}
 
+	// Order of locking:
+	// 1. resizeLock
+	// 2. terminal's lock
+	terminalAlreadyLocked := false
 	if gui.internalResize == false {
 		gui.resizeLock.Lock()
 		defer gui.resizeLock.Unlock()
+		// No need to lock the terminal right away, we can lock it later
+	} else {
+		terminalAlreadyLocked = true
 	}
 
 	gui.logger.Debugf("Initiating GUI resize to %dx%d", width, height)
@@ -268,6 +283,11 @@ func (gui *GUI) resize(w *glfw.Window, width int, height int) {
 		gui.logger.Debugf("Calculating size in cols/rows...")
 		cols, rows := gui.renderer.GetTermSize()
 		gui.logger.Debugf("Resizing internal terminal...")
+		if !terminalAlreadyLocked {
+			gui.terminal.Lock()
+			defer gui.terminal.Unlock()
+			terminalAlreadyLocked = true
+		}
 		if err := gui.terminal.SetSize(cols, rows); err != nil {
 			gui.logger.Errorf("Failed to resize terminal to %d cols, %d rows: %s", cols, rows, err)
 		}
@@ -278,11 +298,16 @@ func (gui *GUI) resize(w *glfw.Window, width int, height int) {
 	gui.logger.Debugf("Setting viewport size...")
 	gl.Viewport(0, 0, int32(gui.width), int32(gui.height))
 
+	if !terminalAlreadyLocked {
+		gui.terminal.Lock()
+		defer gui.terminal.Unlock()
+		terminalAlreadyLocked = true
+	}
 	gui.terminal.SetCharSize(gui.renderer.cellWidth, gui.renderer.cellHeight)
 
 	gui.logger.Debugf("Resize complete!")
 
-	gui.redraw()
+	gui.redraw(!terminalAlreadyLocked)
 	gui.window.SwapBuffers()
 }
 
@@ -337,11 +362,11 @@ func (gui *GUI) Render() error {
 	gui.window.SetMouseButtonCallback(gui.mouseButtonCallback)
 	gui.window.SetCursorPosCallback(gui.mouseMoveCallback)
 	gui.window.SetRefreshCallback(func(w *glfw.Window) {
-		gui.terminal.SetDirty()
+		gui.terminal.SetDirtyLocked()
 	})
 	gui.window.SetFocusCallback(func(w *glfw.Window, focused bool) {
 		if focused {
-			gui.terminal.SetDirty()
+			gui.terminal.SetDirtyLocked()
 		}
 	})
 	gui.window.SetPosCallback(gui.windowPosChangeCallback)
@@ -354,6 +379,10 @@ func (gui *GUI) Render() error {
 		gui.resize(gui.window, w, h)
 	}
 
+	gui.terminal.AttachTitleChangeHandler(titleChan)
+	gui.terminal.AttachResizeHandler(resizeChan)
+	gui.terminal.AttachReverseHandler(reverseChan)
+
 	gui.logger.Debugf("Starting pty read handling...")
 
 	go func() {
@@ -372,10 +401,6 @@ func (gui *GUI) Render() error {
 	gl.Disable(gl.DEPTH_TEST)
 	gl.TexParameterf(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST)
 
-	gui.terminal.AttachTitleChangeHandler(titleChan)
-	gui.terminal.AttachResizeHandler(resizeChan)
-	gui.terminal.AttachReverseHandler(reverseChan)
-
 	ticker := time.NewTicker(time.Second)
 	defer ticker.Stop()
 
@@ -409,8 +434,7 @@ func (gui *GUI) Render() error {
 		case <-titleChan:
 			gui.window.SetTitle(gui.terminal.GetTitle())
 		case <-resizeChan:
-			cols, rows := gui.terminal.GetSize()
+			gui.resizeToTerminal()
-			gui.resizeToTerminal(uint(cols), uint(rows))
 		case reverse := <-reverseChan:
 			gui.generateDefaultCell(reverse)
 			forceRedraw = true
@@ -421,7 +445,7 @@ func (gui *GUI) Render() error {
 
 		if gui.terminal.CheckDirty() || forceRedraw {
 
-			gui.redraw()
+			gui.redraw(true)
 
 			if gui.showDebugInfo {
 				gui.textbox(2, 2, fmt.Sprintf(`Cursor:      %d,%d
@@ -441,7 +465,7 @@ Buffer Size: %d lines
 
 			if showMessage {
 				if latestVersion != "" && time.Since(startTime) < time.Second*10 && gui.terminal.ActiveBuffer().RawLine() == 0 {
-					time.AfterFunc(time.Second, gui.terminal.SetDirty)
+					time.AfterFunc(time.Second, gui.terminal.SetDirtyLocked)
 					_, h := gui.terminal.GetSize()
 					var msg string
 					if version.Version == "" {
@@ -471,8 +495,11 @@ Buffer Size: %d lines
 
 }
 
-func (gui *GUI) redraw() {
+func (gui *GUI) renderTerminalData(shouldLock bool) {
-	gl.Clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT)
+	if shouldLock {
+		gui.terminal.Lock()
+		defer gui.terminal.Unlock()
+	}
 	lines := gui.terminal.GetVisibleLines()
 	lineCount := int(gui.terminal.ActiveBuffer().ViewHeight())
 	colCount := int(gui.terminal.ActiveBuffer().ViewWidth())
@@ -602,6 +629,11 @@ func (gui *GUI) redraw() {
 		}
 
 	}
+}
+
+func (gui *GUI) redraw(shouldLock bool) {
+	gl.Clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT)
+	gui.renderTerminalData(shouldLock)
 	gui.renderOverlay()
 }
 

gui/overlays.go

@@ -5,7 +5,7 @@ type overlay interface {
 }
 
 func (gui *GUI) setOverlay(m overlay) {
-	defer gui.terminal.SetDirty()
+	defer gui.terminal.SetDirtyLocked()
 	gui.overlay = m
 }
 

main_test.go

@@ -105,7 +105,11 @@ func TestCursorMovement(t *testing.T) {
 			send(term, "1\n")
 			sleep()
 
-			if term.ActiveBuffer().CompareViewLines("vttest/test-cursor-movement-1") == false {
+			term.Lock()
+			compareResult := term.ActiveBuffer().CompareViewLines("vttest/test-cursor-movement-1")
+			term.Unlock()
+
+			if compareResult == false {
 				os.Exit(terminate(fmt.Sprintf("ActiveBuffer doesn't match vttest template vttest/test-cursor-movement-1")))
 			}
 

terminal/ansi.go

@@ -35,26 +35,42 @@ func swallowHandler(n int) func(pty chan rune, terminal *Terminal) error {
 }
 
 func risHandler(pty chan rune, terminal *Terminal) error {
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	terminal.ActiveBuffer().Clear()
 	return nil
 }
 
 func indexHandler(pty chan rune, terminal *Terminal) error {
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	terminal.ActiveBuffer().Index()
 	return nil
 }
 
 func reverseIndexHandler(pty chan rune, terminal *Terminal) error {
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	terminal.ActiveBuffer().ReverseIndex()
 	return nil
 }
 
 func saveCursorHandler(pty chan rune, terminal *Terminal) error {
+	// Handler should lock the terminal if there will be write operations to any data read by the renderer
+	// terminal.Lock()
+	// defer terminal.Unlock()
+
 	terminal.ActiveBuffer().SaveCursor()
 	return nil
 }
 
 func restoreCursorHandler(pty chan rune, terminal *Terminal) error {
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	terminal.ActiveBuffer().RestoreCursor()
 	return nil
 }
@@ -73,11 +89,18 @@ func ansiHandler(pty chan rune, terminal *Terminal) error {
 }
 
 func nextLineHandler(pty chan rune, terminal *Terminal) error {
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	terminal.ActiveBuffer().NewLineEx(true)
 	return nil
 }
 
 func tabSetHandler(pty chan rune, terminal *Terminal) error {
+	// Handler should lock the terminal if there will be write operations to any data read by the renderer
+	// terminal.Lock()
+	// defer terminal.Unlock()
+
 	terminal.terminalState.TabSetAtCursor()
 	return nil
 }

terminal/charset.go

@@ -54,6 +54,10 @@ func scs1Handler(pty chan rune, terminal *Terminal) error {
 func scsHandler(pty chan rune, terminal *Terminal, which int) error {
 	b := <-pty
 
+	// Handler should lock the terminal if there will be write operations to any data read by the renderer
+	// terminal.Lock()
+	// defer terminal.Unlock()
+
 	cs, ok := charSets[b]
 	if ok {
 		terminal.logger.Debugf("Selected charset %v into G%v", string(b), which)

terminal/csi.go

@@ -89,6 +89,9 @@ func splitParams(paramString string) []string {
 func csiHandler(pty chan rune, terminal *Terminal) error {
 	final, param, intermediate := loadCSI(pty)
 
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	// process intermediate control codes before the CSI
 	for _, b := range intermediate {
 		terminal.processRune(b)

terminal/output.go

@@ -72,6 +72,13 @@ func shiftInHandler(terminal *Terminal) error {
 	return nil
 }
 
+func (terminal *Terminal) processRuneLocked(b rune) {
+	terminal.Lock()
+	defer terminal.Unlock()
+
+	terminal.processRune(b)
+}
+
 func (terminal *Terminal) processRune(b rune) {
 	if handler, ok := runeMap[b]; ok {
 		if err := handler(terminal); err != nil {
@@ -103,6 +110,8 @@ func (terminal *Terminal) processInput(pty chan rune) {
 
 	var b rune
 
+	//	debug := ""
+
 	for {
 
 		if terminal.config.Slomo {
@@ -111,15 +120,19 @@ func (terminal *Terminal) processInput(pty chan rune) {
 
 		b = <-pty
 
+		// debug += fmt.Sprintf("0x%x ", b)
+
 		if b == 0x1b {
+			// terminal.logger.Debug(debug)
+			// debug = ""
 			//terminal.logger.Debugf("Handling escape sequence: 0x%x", b)
 			if err := ansiHandler(pty, terminal); err != nil {
 				terminal.logger.Errorf("Error handling escape sequence: %s", err)
 			}
-			terminal.isDirty = true
+			terminal.SetDirtyLocked()
 			continue
 		}
 
-		terminal.processRune(b)
+		terminal.processRuneLocked(b)
 	}
 }

terminal/scr_state.go

@@ -6,6 +6,9 @@ func screenStateHandler(pty chan rune, terminal *Terminal) error {
 	b := <-pty
 	switch b {
 	case '8': // DECALN -- Screen Alignment Pattern
+		terminal.Lock()
+		defer terminal.Unlock()
+
 		// hide cursor?
 		buffer := terminal.ActiveBuffer()
 		terminal.ResetVerticalMargins()

terminal/sixel.go

@@ -131,6 +131,9 @@ func sixelHandler(pty chan rune, terminal *Terminal) error {
 		defer terminal.SetLineFeedMode()
 	}
 
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	drawSixel(six, terminal)
 
 	return nil

terminal/terminal.go

@@ -105,6 +105,9 @@ func (terminal *Terminal) SetBracketedPasteMode(enabled bool) {
 }
 
 func (terminal *Terminal) CheckDirty() bool {
+	terminal.Lock()
+	defer terminal.Unlock()
+
 	d := terminal.isDirty
 	terminal.isDirty = false
 	return d || terminal.ActiveBuffer().IsDirty()
@@ -355,9 +358,6 @@ func (terminal *Terminal) GetSize() (int, int) {
 }
 
 func (terminal *Terminal) SetSize(newCols uint, newLines uint) error {
-	terminal.lock.Lock()
-	defer terminal.lock.Unlock()
-
 	if terminal.size.Width == uint16(newCols) && terminal.size.Height == uint16(newLines) {
 		return nil
 	}
@@ -420,3 +420,20 @@ func (terminal *Terminal) SetScreenMode(enabled bool) {
 	}
 	terminal.emitReverse(enabled)
 }
+
+func (terminal *Terminal) Lock() {
+	terminal.lock.Lock()
+}
+
+func (terminal *Terminal) Unlock() {
+	terminal.lock.Unlock()
+}
+
+// SetDirtyLocked sets dirty flag locking the terminal to prevent data race warnings
+// @todo remove when switching to event-driven architecture
+func (terminal *Terminal) SetDirtyLocked() {
+	terminal.Lock()
+	defer terminal.Unlock()
+
+	terminal.SetDirty()
+}