diff --git a/client_notification_test.go b/client_notification_test.go index d664756..133642e 100644 --- a/client_notification_test.go +++ b/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"} diff --git a/fix.txt b/fix.txt index 938dd31..30b676f 100644 --- a/fix.txt +++ b/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. diff --git a/main.go b/main.go index 29b8dbc..aa73ca5 100644 --- a/main.go +++ b/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 { - event := <-events + for event := range 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 == "" { diff --git a/ui.go b/ui.go index 1bb7b49..1aee71b 100644 --- a/ui.go +++ b/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) {