ui: fix deadlocks in message channel

There are several ways the ui message channel can fill up leading to
deadlocks.

1) Invalidate() changes the value of uiState to DIRTY. The following
   call sequence:

      QueueRedraw()
      Invalidate()
      QueueRedraw()

   Leads to multiple nil messages being queued in the message channel
   whereas one could assume that the second QueueRedraw() would do
   nothing. This is caused by the tri-state nature of uiState.

2) We use the same channel to convey state change, keyboard events and
   redraw requests. Since a keyboard event almost always triggers
   a redraw, we end up trying to append a redraw message in the same
   goroutine that reads from the channel. This triggers a deadlock when
   there are more than 50 pending messages.

Solve the issue by using multiple channels, one per type of message that
needs to be sent to the main ui thread.

Remove QueueRedraw() and merge its functionality in Invalidate(). Only
use a DIRTY/CLEAN state to determine if something needs to be queued in
the redraw channel.

Use a channel for quitting instead of an atomic. Restructure some code
functions to have a cleaner API.

Use a for loop in the main thread and select from all channels.

Signed-off-by: Robin Jarry <robin@jarry.cc>
Tested-by: Koni Marti <koni.marti@gmail.com>
Tested-by: Maarten van Gompel <proycon@anaproy.nl>

10 files changed, 46 insertions, 70 deletions

diff --git a/commands/term.go b/commands/term.go
index 64657747..22957635 100644
--- a/commands/term.go
+++ b/commands/term.go
@@ -43,7 +43,7 @@ func TermCore(aerc *widgets.Aerc, args []string) error {
 		}
 		if tab.Name != title {
 			tab.Name = title
-			ui.QueueRedraw()
+			ui.Invalidate()
 		}
 	}
 	term.OnClose = func(err error) {
diff --git a/lib/msgstore.go b/lib/msgstore.go
index 7cdc5810..29b3c53c 100644
--- a/lib/msgstore.go
+++ b/lib/msgstore.go
@@ -427,7 +427,7 @@ func (store *MessageStore) runThreadBuilder() {
 	}
 	store.threadBuilderDebounce = time.AfterFunc(store.threadBuilderDelay, func() {
 		store.runThreadBuilderNow()
-		ui.QueueRedraw()
+		ui.Invalidate()
 	})
 }
 
diff --git a/lib/ui/interfaces.go b/lib/ui/interfaces.go
index 8ede22c7..19f0c04f 100644
--- a/lib/ui/interfaces.go
+++ b/lib/ui/interfaces.go
@@ -4,9 +4,6 @@ import (
 	"github.com/gdamore/tcell/v2"
 )
 
-// AercMsg is used to communicate within aerc
-type AercMsg interface{}
-
 // 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 {
diff --git a/lib/ui/textinput.go b/lib/ui/textinput.go
index d76e9349..b2ba33f6 100644
--- a/lib/ui/textinput.go
+++ b/lib/ui/textinput.go
@@ -329,7 +329,7 @@ func (ti *TextInput) showCompletions() {
 	}
 	ti.completions, ti.prefix = ti.tabcomplete(ti.StringLeft())
 	ti.completeIndex = -1
-	QueueRedraw()
+	Invalidate()
 }
 
 func (ti *TextInput) OnChange(onChange func(ti *TextInput)) {
diff --git a/lib/ui/ui.go b/lib/ui/ui.go
index bc054ac5..628946fa 100644
--- a/lib/ui/ui.go
+++ b/lib/ui/ui.go
@@ -3,52 +3,43 @@ package ui
 import (
 	"sync/atomic"
 
-	"git.sr.ht/~rjarry/aerc/log"
 	"github.com/gdamore/tcell/v2"
 )
 
 const (
 	// nominal state, UI is up to date
 	CLEAN int32 = iota
-	// redraw is required but not explicitly requested
+	// UI render has been queued in Redraw channel
 	DIRTY
-	// redraw has been explicitly requested
-	REDRAW_PENDING
 )
 
-var MsgChannel = make(chan AercMsg, 50)
-
-type AercFuncMsg struct {
-	Func func()
-}
-
 // State of the UI. Any value other than 0 means the UI is in a dirty state.
 // This should only be accessed via atomic operations to maintain thread safety
 var uiState int32
 
-// QueueRedraw marks the UI as invalid and sends a nil message into the
-// MsgChannel. Nothing will handle this message, but a redraw will occur
-func QueueRedraw() {
-	if atomic.SwapInt32(&uiState, REDRAW_PENDING) != REDRAW_PENDING {
-		MsgChannel <- nil
-	}
-}
+var Callbacks = make(chan func(), 50)
 
 // QueueFunc queues a function to be called in the main goroutine. This can be
 // used to prevent race conditions from delayed functions
 func QueueFunc(fn func()) {
-	MsgChannel <- &AercFuncMsg{Func: fn}
+	Callbacks <- fn
 }
 
-// Invalidate marks the entire UI as invalid. Invalidate can be called from any
-// goroutine
+// Use a buffered channel of size 1 to avoid blocking callers of Invalidate()
+var Redraw = make(chan bool, 1)
+
+// Invalidate marks the entire UI as invalid and request a redraw as soon as
+// possible. Invalidate can be called from any goroutine and will never block.
 func Invalidate() {
-	atomic.StoreInt32(&uiState, DIRTY)
+	if atomic.SwapInt32(&uiState, DIRTY) != DIRTY {
+		Redraw <- true
+	}
 }
 
 type UI struct {
 	Content DrawableInteractive
-	exit    atomic.Value // bool
+	Quit    chan struct{}
+	Events  chan tcell.Event
 	ctx     *Context
 	screen  tcell.Screen
 	popover *Popover
@@ -73,11 +64,14 @@ func Initialize(content DrawableInteractive) (*UI, error) {
 	state := UI{
 		Content: content,
 		screen:  screen,
+		// Use unbuffered channels (always blocking unless somebody can
+		// read immediately) We are merely using this as a proxy to
+		// tcell screen internal event channel.
+		Events: make(chan tcell.Event),
+		Quit:   make(chan struct{}),
 	}
 	state.ctx = NewContext(width, height, screen, state.onPopover)
 
-	state.exit.Store(false)
-
 	Invalidate()
 	if beeper, ok := content.(DrawableInteractiveBeeper); ok {
 		beeper.OnBeep(screen.Beep)
@@ -87,6 +81,7 @@ func Initialize(content DrawableInteractive) (*UI, error) {
 	if root, ok := content.(RootDrawable); ok {
 		root.Initialize(&state)
 	}
+	go state.screen.ChannelEvents(state.Events, state.Quit)
 
 	return &state, nil
 }
@@ -95,12 +90,8 @@ func (state *UI) onPopover(p *Popover) {
 	state.popover = p
 }
 
-func (state *UI) ShouldExit() bool {
-	return state.exit.Load().(bool)
-}
-
 func (state *UI) Exit() {
-	state.exit.Store(true)
+	close(state.Quit)
 }
 
 func (state *UI) Close() {
@@ -124,15 +115,6 @@ func (state *UI) EnableMouse() {
 	state.screen.EnableMouse()
 }
 
-func (state *UI) ChannelEvents() {
-	go func() {
-		defer log.PanicHandler()
-		for {
-			MsgChannel <- state.screen.PollEvent()
-		}
-	}()
-}
-
 func (state *UI) HandleEvent(event tcell.Event) {
 	if event, ok := event.(*tcell.EventResize); ok {
 		state.screen.Clear()
diff --git a/main.go b/main.go
index a403c751..4705c438 100644
--- a/main.go
+++ b/main.go
@@ -12,7 +12,6 @@ import (
 	"time"
 
 	"git.sr.ht/~sircmpwn/getopt"
-	"github.com/gdamore/tcell/v2"
 	"github.com/mattn/go-isatty"
 	"github.com/xo/terminfo"
 
@@ -251,7 +250,6 @@ func main() {
 		setWindowTitle()
 	}
 
-	ui.ChannelEvents()
 	go func() {
 		defer log.PanicHandler()
 		err := hooks.RunHook(&hooks.AercStartup{Version: Version})
@@ -268,22 +266,23 @@ func main() {
 			log.Errorf("aerc-shutdown hook: %s", err)
 		}
 	}(time.Now())
-	for event := range libui.MsgChannel {
-		switch event := event.(type) {
-		case tcell.Event:
+loop:
+	for {
+		select {
+		case event := <-ui.Events:
 			ui.HandleEvent(event)
-		case *libui.AercFuncMsg:
-			event.Func()
-		case types.WorkerMessage:
-			aerc.HandleMessage(event)
-		}
-		if ui.ShouldExit() {
-			break
+		case msg := <-types.WorkerMessages:
+			aerc.HandleMessage(msg)
+		case callback := <-libui.Callbacks:
+			callback()
+		case <-libui.Redraw:
+			ui.Render()
+		case <-ui.Quit:
+			err = aerc.CloseBackends()
+			if err != nil {
+				log.Warnf("failed to close backends: %v", err)
+			}
+			break loop
 		}
-		ui.Render()
-	}
-	err = aerc.CloseBackends()
-	if err != nil {
-		log.Warnf("failed to close backends: %v", err)
 	}
 }
diff --git a/widgets/aerc.go b/widgets/aerc.go
index 9f88ecb6..85ce686b 100644
--- a/widgets/aerc.go
+++ b/widgets/aerc.go
@@ -732,7 +732,6 @@ func (aerc *Aerc) Mailto(addr *url.URL) error {
 	}
 
 	defer ui.Invalidate()
-	defer ui.QueueRedraw()
 
 	composer, err := NewComposer(aerc, acct,
 		acct.AccountConfig(), acct.Worker(), template, h, nil)
@@ -775,7 +774,6 @@ func (aerc *Aerc) Mbox(source string) error {
 	acctConf.CopyTo = "Sent"
 
 	defer ui.Invalidate()
-	defer ui.QueueRedraw()
 
 	mboxView, err := NewAccountView(aerc, &acctConf, aerc, nil)
 	if err != nil {
@@ -788,7 +786,6 @@ func (aerc *Aerc) Mbox(source string) error {
 }
 
 func (aerc *Aerc) Command(args []string) error {
-	defer ui.QueueRedraw()
 	defer ui.Invalidate()
 	return aerc.cmd(args, nil, nil)
 }
diff --git a/widgets/spinner.go b/widgets/spinner.go
index abbe1ae4..63eaf11b 100644
--- a/widgets/spinner.go
+++ b/widgets/spinner.go
@@ -49,7 +49,7 @@ func (s *Spinner) Start() {
 				return
 			case <-time.After(s.interval):
 				atomic.AddInt64(&s.frame, 1)
-				ui.QueueRedraw()
+				ui.Invalidate()
 			}
 		}
 	}()
diff --git a/widgets/terminal.go b/widgets/terminal.go
index 9c2123cc..cf0888ee 100644
--- a/widgets/terminal.go
+++ b/widgets/terminal.go
@@ -58,7 +58,7 @@ func (term *Terminal) closeErr(err error) {
 		term.ctx.HideCursor()
 	}
 	term.closed = true
-	ui.QueueRedraw()
+	ui.Invalidate()
 }
 
 func (term *Terminal) Destroy() {
@@ -165,7 +165,7 @@ func (term *Terminal) HandleEvent(ev tcell.Event) {
 	switch ev := ev.(type) {
 	case *tcellterm.EventRedraw:
 		if term.visible {
-			ui.QueueRedraw()
+			ui.Invalidate()
 		}
 	case *tcellterm.EventTitle:
 		if term.OnTitle != nil {
@@ -173,7 +173,7 @@ func (term *Terminal) HandleEvent(ev tcell.Event) {
 		}
 	case *tcellterm.EventClosed:
 		term.Close()
-		ui.QueueRedraw()
+		ui.Invalidate()
 	}
 }
 
diff --git a/worker/types/worker.go b/worker/types/worker.go
index 7033bf77..b1927b90 100644
--- a/worker/types/worker.go
+++ b/worker/types/worker.go
@@ -5,7 +5,6 @@ import (
 	"sync"
 	"sync/atomic"
 
-	"git.sr.ht/~rjarry/aerc/lib/ui"
 	"git.sr.ht/~rjarry/aerc/log"
 	"git.sr.ht/~rjarry/aerc/models"
 )
@@ -101,6 +100,8 @@ func (worker *Worker) PostAction(msg WorkerMessage, cb func(msg WorkerMessage))
 	}
 }
 
+var WorkerMessages = make(chan WorkerMessage, 50)
+
 // PostMessage posts an message to the UI. This method should not be called
 // from the same goroutine that the UI runs in or deadlocks may occur
 func (worker *Worker) PostMessage(msg WorkerMessage,
@@ -114,7 +115,7 @@ func (worker *Worker) PostMessage(msg WorkerMessage,
 	} else {
 		log.Tracef("(%s) PostMessage %T", worker.Name, msg)
 	}
-	ui.MsgChannel <- msg
+	WorkerMessages <- msg
 
 	if cb != nil {
 		worker.Lock()