Update internal state and draw from the same goroutine

This commit introduces a new Aerc.Tick function that should be called to
refresh the internal state. This in turn makes each AccountView process worker
events.

The UI goroutine repeatedly refreshes the internal state before drawing a new
frame. The reason for this is that many worker messages may need to be
processed for a single frame, and drawing the UI is far slower than refreshing
the internal state. This has been confirmed in my testing (calling Aerc.Tick
only once per frame results in a slower display).

Many synchronization code has been removed. We can now write widgets without
having to care so much about races. The remaining sync users are:

- widgets/spinner: the spinner value is updated from inside an internal
  goroutine
- lib/ui/invalidatable: Invalidate may be called from any goroutine
- lib/ui/grid: same
- lib/ui/ui: an internal goroutine needs read access to UI.exit
- worker/types/worker: Worker.callbacks is used for both worker and UI
  callbacks

The exact goroutine requirements for Drawable have been documented.

aerc.go

@@ -99,6 +99,9 @@ func main() {
 	defer ui.Close()
 
 	for !ui.ShouldExit() {
+		for aerc.Tick() {
+			// Continue updating our internal state
+		}
 		if !ui.Tick() {
 			// ~60 FPS
 			time.Sleep(16 * time.Millisecond)

lib/msgstore.go

@@ -2,7 +2,6 @@ package lib
 
 import (
 	"io"
-	"sync"
 	"time"
 
 	"github.com/emersion/go-imap"
@@ -12,8 +11,6 @@ import (
 
 // Accesses to fields must be guarded by MessageStore.Lock/Unlock
 type MessageStore struct {
-	sync.Mutex
-
 	Deleted  map[uint32]interface{}
 	DirInfo  types.DirectoryInfo
 	Messages map[uint32]*types.MessageInfo
@@ -49,9 +46,6 @@ func NewMessageStore(worker *types.Worker,
 func (store *MessageStore) FetchHeaders(uids []uint32,
 	cb func(*types.MessageInfo)) {
 
-	store.Lock()
-	defer store.Unlock()
-
 	// TODO: this could be optimized by pre-allocating toFetch and trimming it
 	// at the end. In practice we expect to get most messages back in one frame.
 	var toFetch imap.SeqSet
@@ -74,9 +68,6 @@ func (store *MessageStore) FetchHeaders(uids []uint32,
 }
 
 func (store *MessageStore) FetchFull(uids []uint32, cb func(io.Reader)) {
-	store.Lock()
-	defer store.Unlock()
-
 	// TODO: this could be optimized by pre-allocating toFetch and trimming it
 	// at the end. In practice we expect to get most messages back in one frame.
 	var toFetch imap.SeqSet
@@ -134,8 +125,6 @@ func merge(to *types.MessageInfo, from *types.MessageInfo) {
 }
 
 func (store *MessageStore) Update(msg types.WorkerMessage) {
-	store.Lock()
-
 	update := false
 	switch msg := msg.(type) {
 	case *types.DirectoryInfo:
@@ -201,8 +190,6 @@ func (store *MessageStore) Update(msg types.WorkerMessage) {
 		update = true
 	}
 
-	store.Unlock()
-
 	if update {
 		store.update()
 	}
@@ -220,7 +207,6 @@ func (store *MessageStore) update() {
 
 func (store *MessageStore) Delete(uids []uint32,
 	cb func(msg types.WorkerMessage)) {
-	store.Lock()
 
 	var set imap.SeqSet
 	for _, uid := range uids {
@@ -228,8 +214,6 @@ func (store *MessageStore) Delete(uids []uint32,
 		store.Deleted[uid] = nil
 	}
 
-	store.Unlock()
-
 	store.worker.PostAction(&types.DeleteMessages{Uids: set}, cb)
 	store.update()
 }
@@ -249,7 +233,6 @@ func (store *MessageStore) Copy(uids []uint32, dest string,
 
 func (store *MessageStore) Move(uids []uint32, dest string,
 	cb func(msg types.WorkerMessage)) {
-	store.Lock()
 
 	var set imap.SeqSet
 	for _, uid := range uids {
@@ -257,8 +240,6 @@ func (store *MessageStore) Move(uids []uint32, dest string,
 		store.Deleted[uid] = nil
 	}
 
-	store.Unlock()
-
 	store.worker.PostAction(&types.CopyMessages{
 		Destination: dest,
 		Uids:        set,

lib/ui/interfaces.go

@@ -4,12 +4,15 @@ import (
 	"github.com/gdamore/tcell"
 )
 
+// Drawable is a UI component that can draw. Unless specified, all methods must
+// only be called from a single goroutine, the UI goroutine.
 type Drawable interface {
-	// Called when this renderable should draw itself
+	// Called when this renderable should draw itself.
 	Draw(ctx *Context)
-	// Specifies a function to call when this cell needs to be redrawn
+	// Specifies a function to call when this cell needs to be redrawn. The
+	// callback may be called in any goroutine.
 	OnInvalidate(callback func(d Drawable))
-	// Invalidates the drawable
+	// Invalidates the drawable. This can be called from any goroutine.
 	Invalidate()
 }
 

lib/ui/ui.go

@@ -10,7 +10,7 @@ import (
 
 type UI struct {
 	Content DrawableInteractive
-	exit    atomic.Value
+	exit    atomic.Value // bool
 	ctx     *Context
 	screen  tcell.Screen
 

widgets/account.go

@@ -65,13 +65,6 @@ func NewAccountView(conf *config.AercConfig, acct *config.AccountConfig,
 	}
 
 	go worker.Backend.Run()
-	go func() {
-		for {
-			msg := <-worker.Messages
-			msg = worker.ProcessMessage(msg)
-			view.onMessage(msg)
-		}
-	}()
 
 	worker.PostAction(&types.Configure{Config: acct}, nil)
 	worker.PostAction(&types.Connect{}, view.connected)
@@ -80,6 +73,17 @@ func NewAccountView(conf *config.AercConfig, acct *config.AccountConfig,
 	return view
 }
 
+func (acct *AccountView) Tick() bool {
+	select {
+	case msg := <-acct.worker.Messages:
+		msg = acct.worker.ProcessMessage(msg)
+		acct.onMessage(msg)
+		return true
+	default:
+		return false
+	}
+}
+
 func (acct *AccountView) AccountConfig() *config.AccountConfig {
 	return acct.acct
 }

widgets/aerc.go

@@ -65,6 +65,14 @@ func NewAerc(conf *config.AercConfig, logger *log.Logger,
 	return aerc
 }
 
+func (aerc *Aerc) Tick() bool {
+	more := false
+	for _, acct := range aerc.accounts {
+		more = acct.Tick() || more
+	}
+	return more
+}
+
 func (aerc *Aerc) Children() []ui.Drawable {
 	return aerc.grid.Children()
 }

widgets/msglist.go

@@ -2,7 +2,6 @@ package widgets
 
 import (
 	"log"
-	"sync/atomic"
 
 	"github.com/gdamore/tcell"
 
@@ -20,7 +19,7 @@ type MessageList struct {
 	scroll   int
 	selected int
 	spinner  *Spinner
-	store    atomic.Value // *lib.MessageStore
+	store    *lib.MessageStore
 }
 
 func NewMessageList(conf *config.AercConfig, logger *log.Logger) *MessageList {
@@ -30,7 +29,6 @@ func NewMessageList(conf *config.AercConfig, logger *log.Logger) *MessageList {
 		selected: 0,
 		spinner:  NewSpinner(),
 	}
-	ml.store.Store((*lib.MessageStore)(nil))
 	ml.spinner.OnInvalidate(func(_ ui.Drawable) {
 		ml.Invalidate()
 	})
@@ -53,8 +51,6 @@ func (ml *MessageList) Draw(ctx *ui.Context) {
 		return
 	}
 
-	store.Lock()
-
 	var (
 		needsHeaders []uint32
 		row          int = 0
@@ -94,8 +90,6 @@ func (ml *MessageList) Draw(ctx *ui.Context) {
 			tcell.StyleDefault, "%s", msg)
 	}
 
-	store.Unlock()
-
 	if len(needsHeaders) != 0 {
 		store.FetchHeaders(needsHeaders, nil)
 		ml.spinner.Start()
@@ -113,13 +107,11 @@ func (ml *MessageList) storeUpdate(store *lib.MessageStore) {
 		return
 	}
 
-	store.Lock()
 	if len(store.Uids) > 0 {
 		for ml.selected >= len(store.Uids) {
 			ml.Prev()
 		}
 	}
-	store.Unlock()
 
 	ml.Invalidate()
 }
@@ -129,7 +121,7 @@ func (ml *MessageList) SetStore(store *lib.MessageStore) {
 		ml.scroll = 0
 		ml.selected = 0
 	}
-	ml.store.Store(store)
+	ml.store = store
 	if store != nil {
 		ml.spinner.Stop()
 		store.OnUpdate(ml.storeUpdate)
@@ -140,29 +132,21 @@ func (ml *MessageList) SetStore(store *lib.MessageStore) {
 }
 
 func (ml *MessageList) Store() *lib.MessageStore {
-	return ml.store.Load().(*lib.MessageStore)
+	return ml.store
 }
 
 func (ml *MessageList) Empty() bool {
 	store := ml.Store()
-	store.Lock()
-	defer store.Unlock()
-
 	return store == nil || len(store.Uids) == 0
 }
 
 func (ml *MessageList) Selected() *types.MessageInfo {
 	store := ml.Store()
-	store.Lock()
-	defer store.Unlock()
-
 	return store.Messages[store.Uids[len(store.Uids)-ml.selected-1]]
 }
 
 func (ml *MessageList) Select(index int) {
 	store := ml.Store()
-	store.Lock()
-	defer store.Unlock()
 
 	ml.selected = index
 	for ; ml.selected < 0; ml.selected = len(store.Uids) + ml.selected {
@@ -181,8 +165,6 @@ func (ml *MessageList) Select(index int) {
 
 func (ml *MessageList) nextPrev(delta int) {
 	store := ml.Store()
-	store.Lock()
-	defer store.Unlock()
 
 	if store == nil || len(store.Uids) == 0 {
 		return