summary refs log tree commit diff stats
path: root/lib
diff options
context:
space:
mode:
authorSimon Ser <contact@emersion.fr>2019-05-04 14:13:57 +0000
committerDrew DeVault <sir@cmpwn.com>2019-05-05 01:06:39 -0400
commit5feb7dede93900b0012e3047985d9a1739f3ed5d (patch)
treeafe20d99ee66dab3daeffdeb5feaf06faa702cb2 /lib
parenta275f65848f2c1fdd4302f56121defc408e7d8b6 (diff)
downloadaerc-5feb7dede93900b0012e3047985d9a1739f3ed5d.tar.gz
lib/ui: fix Grid race condition
This was is more complicated than others. The cells list is accessed by
multiple goroutines:

- Some change the Grid's contents via AddChild/RemoveChild
- Some call Draw
- Some invalidate the grid via Invalidate

Invalidate calls are tricky to handle because they will also invalidate all
child cells. This will inturn trigger the cellInvalidated callback, which needs
to read the list of cells.

For this reason, we use a sync.RWLock which allows multiple concurrent reads.

Below is the race fixed by this commit.

    Read at 0x00c0000bc3d0 by goroutine 7:
      git.sr.ht/~sircmpwn/aerc2/lib/ui.(*Grid).cellInvalidated()
          /home/simon/src/aerc2/lib/ui/grid.go:181 +0x45
      git.sr.ht/~sircmpwn/aerc2/lib/ui.(*Grid).cellInvalidated-fm()
          /home/simon/src/aerc2/lib/ui/grid.go:179 +0x55
      git.sr.ht/~sircmpwn/aerc2/lib/ui.(*Invalidatable).DoInvalidate()
          /home/simon/src/aerc2/lib/ui/invalidatable.go:22 +0x85
      git.sr.ht/~sircmpwn/aerc2/lib/ui.(*Bordered).contentInvalidated-fm()
          /home/simon/src/aerc2/lib/ui/borders.go:39 +0x56
      git.sr.ht/~sircmpwn/aerc2/lib/ui.(*Invalidatable).DoInvalidate()
          /home/simon/src/aerc2/lib/ui/invalidatable.go:22 +0x85
      git.sr.ht/~sircmpwn/aerc2/widgets.NewDirectoryList.func1()
          /home/simon/src/aerc2/widgets/dirlist.go:81 +0x55
      git.sr.ht/~sircmpwn/aerc2/lib/ui.(*Invalidatable).DoInvalidate()
          /home/simon/src/aerc2/lib/ui/invalidatable.go:22 +0x85
      git.sr.ht/~sircmpwn/aerc2/widgets.(*Spinner).Start.func1()
          /home/simon/src/aerc2/widgets/spinner.go:88 +0x82

    Previous write at 0x00c0000bc3d0 by main goroutine:
      [failed to restore the stack]

    Goroutine 7 (running) created at:
      git.sr.ht/~sircmpwn/aerc2/widgets.(*Spinner).Start()
          /home/simon/src/aerc2/widgets/spinner.go:46 +0x98
      git.sr.ht/~sircmpwn/aerc2/widgets.NewDirectoryList()
          /home/simon/src/aerc2/widgets/dirlist.go:37 +0x28b
      git.sr.ht/~sircmpwn/aerc2/widgets.NewAccountView()
          /home/simon/src/aerc2/widgets/account.go:49 +0x5ca
      git.sr.ht/~sircmpwn/aerc2/widgets.NewAerc()
          /home/simon/src/aerc2/widgets/aerc.go:60 +0x807
      main.main()
          /home/simon/src/aerc2/aerc.go:65 +0x33e
Diffstat (limited to 'lib')
-rw-r--r--lib/ui/grid.go21
1 files changed, 20 insertions, 1 deletions
diff --git a/lib/ui/grid.go b/lib/ui/grid.go
index 3fc7f04..f36a16a 100644
--- a/lib/ui/grid.go
+++ b/lib/ui/grid.go
@@ -3,6 +3,7 @@ package ui
 import (
 	"fmt"
 	"math"
+	"sync"
 	"sync/atomic"
 )
 
@@ -12,8 +13,11 @@ type Grid struct {
 	rowLayout    []gridLayout
 	columns      []GridSpec
 	columnLayout []gridLayout
-	cells        []*GridCell
 	invalid      bool
+
+	// Protected by mutex
+	cells []*GridCell
+	mutex sync.RWMutex
 }
 
 const (
@@ -73,6 +77,9 @@ func (grid *Grid) Columns(spec []GridSpec) *Grid {
 }
 
 func (grid *Grid) Children() []Drawable {
+	grid.mutex.RLock()
+	defer grid.mutex.RUnlock()
+
 	children := make([]Drawable, len(grid.cells))
 	for i, cell := range grid.cells {
 		children[i] = cell.Content
@@ -85,6 +92,10 @@ func (grid *Grid) Draw(ctx *Context) {
 	if invalid {
 		grid.reflow(ctx)
 	}
+
+	grid.mutex.RLock()
+	defer grid.mutex.RUnlock()
+
 	for _, cell := range grid.cells {
 		cellInvalid := cell.invalid.Load().(bool)
 		if !cellInvalid && !invalid {
@@ -148,9 +159,11 @@ func (grid *Grid) invalidateLayout() {
 
 func (grid *Grid) Invalidate() {
 	grid.invalidateLayout()
+	grid.mutex.RLock()
 	for _, cell := range grid.cells {
 		cell.Content.Invalidate()
 	}
+	grid.mutex.RUnlock()
 }
 
 func (grid *Grid) AddChild(content Drawable) *GridCell {
@@ -159,7 +172,9 @@ func (grid *Grid) AddChild(content Drawable) *GridCell {
 		ColSpan: 1,
 		Content: content,
 	}
+	grid.mutex.Lock()
 	grid.cells = append(grid.cells, cell)
+	grid.mutex.Unlock()
 	cell.Content.OnInvalidate(grid.cellInvalidated)
 	cell.invalid.Store(true)
 	grid.invalidateLayout()
@@ -167,23 +182,27 @@ func (grid *Grid) AddChild(content Drawable) *GridCell {
 }
 
 func (grid *Grid) RemoveChild(cell *GridCell) {
+	grid.mutex.Lock()
 	for i, _cell := range grid.cells {
 		if _cell == cell {
 			grid.cells = append(grid.cells[:i], grid.cells[i+1:]...)
 			break
 		}
 	}
+	grid.mutex.Unlock()
 	grid.invalidateLayout()
 }
 
 func (grid *Grid) cellInvalidated(drawable Drawable) {
 	var cell *GridCell
+	grid.mutex.RLock()
 	for _, cell = range grid.cells {
 		if cell.Content == drawable {
 			break
 		}
 		cell = nil
 	}
+	grid.mutex.RUnlock()
 	if cell == nil {
 		panic(fmt.Errorf("Attempted to invalidate unknown cell"))
 	}