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
MessageStore has a lot of exported fields that can be read from the outside.
Each read must be protected, because a call from Update could happen at any
time.
Many Drawable implementations have their own Invalidate and OnInvalidate
functions, with an unexported onInvalidate field. However OnInvalidate and
Invalidate are usually not called in the same goroutine. This results in a race
on this field, e.g.:
Read at 0x00c000094748 by goroutine 7:
git.sr.ht/~sircmpwn/aerc2/widgets.NewDirectoryList.func1()
/home/simon/src/aerc2/widgets/dirlist.go:85 +0x56
git.sr.ht/~sircmpwn/aerc2/widgets.(*Spinner).Start.func1()
/home/simon/src/aerc2/widgets/spinner.go:93 +0x1bb
Previous write at 0x00c000094748 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 +0x8f
git.sr.ht/~sircmpwn/aerc2/widgets.NewDirectoryList()
/home/simon/src/aerc2/widgets/dirlist.go:37 +0x286
git.sr.ht/~sircmpwn/aerc2/widgets.NewAccountView()
/home/simon/src/aerc2/widgets/account.go:50 +0x5ca
git.sr.ht/~sircmpwn/aerc2/widgets.NewAerc()
/home/simon/src/aerc2/widgets/aerc.go:60 +0x800
main.main()
/home/simon/src/aerc2/aerc.go:65 +0x33e
To fix this, introduce a new type, Invalidatable, which protects the field.
Unfortunately the Drawable must be passed to the callback function in
Invalidate, so we still need to re-implement this in each Invalidatable user.
Fix a few potential out of bounds by placing proper checks, which should
be relevant if all tabs are removed for some reason.
Also avoid iterating all tabs in the invalidate handler, since we are
only interested in whether it's the selected tab either way
This is a simple mostly straight forward switch to tcell in favor of
termbox.
It uses the tcell native api (not the compat layer) but does not make
use of most features.
Further changes should include moving to tcell's views.TextArea and the
general built in widget behaviour instead of the current ad hoc
implementation.
Regression: Cursor isn't shown in ex-line
Drew DeVault
Simon Ser
Markus Ongyerth