Bound notification delivery and expansion

client_notification_test.go

@@ -4,6 +4,7 @@ import (
 	"io"
 	"strings"
 	"testing"
+	"time"
 
 	"git.stormux.org/storm/barnard/gumble/gumble"
 )
@@ -34,6 +35,24 @@ func TestReadFIFOStopsOnEOF(t *testing.T) {
 	}
 }
 
+// Regression: sequential substitutions re-expanded placeholders embedded in
+// server-provided fields, and a slow notifier blocked callback goroutines.
+func TestNotificationExpansionIsSinglePassAndNotifyDoesNotBlock(t *testing.T) {
+	got := expandNotification("%event %what", []string{"event", "who", "%event"})
+	if got != "event %event" {
+		t.Fatalf("unexpected expansion %q", got)
+	}
+	b := &Barnard{notifyChannel: make(chan []string, 1)}
+	b.Notify("one", "", "")
+	done := make(chan struct{})
+	go func() { b.Notify("two", "", ""); close(done) }()
+	select {
+	case <-done:
+	case <-time.After(time.Second):
+		t.Fatal("Notify blocked on a full queue")
+	}
+}
+
 func TestUserChangeNotification(t *testing.T) {
 	current := &gumble.Channel{ID: 1, Name: "Current"}
 	other := &gumble.Channel{ID: 2, Name: "Other"}

fix.txt

@@ -136,7 +136,7 @@ Priority 1: transport, lifecycle, and correctness
     lists or strand goroutines. Use mutex-protected listener snapshots and an
     idempotent detach operation.
 
-17. Notification commands block callers and substitution is unsafe
+[x] 17. Notification commands block callers and substitution is unsafe
     File: main.go
     Notify sends to an unbuffered channel. The one consumer waits for each
     shell command, so slow notification programs block UI/network callbacks.

main.go

@@ -47,8 +47,10 @@ func do_list_devices() {
 	show_devs("Inputs:", idevs)
 }
 
+const notificationQueueSize = 32
+
 func setup_notify_runner(notify_command string) chan []string {
-	t := make(chan []string)
+	t := make(chan []string, notificationQueueSize)
 	var do_nothing = false
 	var err error
 	if err != nil {
@@ -57,23 +59,28 @@ func setup_notify_runner(notify_command string) chan []string {
 		do_nothing = true
 	}
 	go func(events chan []string, cmd_template string, dummy bool) {
-		for {
+		for event := range events {
-			event := <-events
 			if !dummy {
-				t := string(cmd_template)
-				t = strings.ReplaceAll(t, "%event", shellescape.Quote(event[0]))
-				t = strings.ReplaceAll(t, "%who", shellescape.Quote(event[1]))
-				t = strings.ReplaceAll(t, "%what", shellescape.Quote(event[2]))
 				cmd := "/bin/sh"
-				args := []string{"-c", t}
+				args := []string{"-c", expandNotification(cmd_template, event)}
 				x := exec.Command(cmd, args...)
 				x.Run()
-			} //if we actually have a command to run
+			}
-		} //for
+		}
 	}(t, notify_command, do_nothing)
 	return t
 }
 
+// expandNotification replaces placeholders in one pass so text supplied for
+// one field cannot cause another placeholder to be expanded recursively.
+func expandNotification(template string, event []string) string {
+	return strings.NewReplacer(
+		"%event", shellescape.Quote(event[0]),
+		"%who", shellescape.Quote(event[1]),
+		"%what", shellescape.Quote(event[2]),
+	).Replace(template)
+}
+
 func setup_fifo(fn string) (chan string, error) {
 	t := make(chan string)
 	if fn == "" {

ui.go

@@ -38,7 +38,12 @@ func esc(str string) string {
 }
 
 func (b *Barnard) Notify(event string, who string, what string) {
-	b.notifyChannel <- []string{event, who, what}
+	// Notifications are best-effort: a slow external command must not block a
+	// UI or network callback. New events are dropped once the bounded queue is full.
+	select {
+	case b.notifyChannel <- []string{event, who, what}:
+	default:
+	}
 }
 
 func (b *Barnard) SetSelectedUser(user *gumble.User) {