From 872149c977c17c227c6e465c96ef1d797d56d7eb Mon Sep 17 00:00:00 2001 From: Brandon McGinty Date: Thu, 20 Aug 2026 14:38:41 -0400 Subject: [PATCH] run all terminal work on the UI goroutine and lock shared client state Route network and audio callbacks through a bounded UI queue. Protocol handlers, the audio thread, and key handlers all drew to termbox widgets directly, which is a data race against the render loop. postUI is now the only path from a callback to a widget, work is dropped during shutdown, and the queue never blocks the caller. Guard the mutable client state with mutexes. Connection and transmission flags, the selected user, the muted channel set, and the audio stream pointer were each read and written from at least two goroutines. Lookups that walk the client's user and channel maps take the client lock and copy what they need. Snapshot the display string when a tree item is built. Tree items held live pointers and formatted themselves during rendering, so a user removed between rebuild and draw was dereferenced on the render path. Cancel reconnect retries on shutdown and release audio before reconnecting. Quitting during a retry left the goroutine sleeping until its timer expired, and a reconnect built a second OpenAL stream on top of the first. Cleanup is idempotent so repeated disconnect events are safe. Make UI shutdown idempotent and join the event poller. Close could be called more than once, and the polling goroutine could be left trying to deliver an event after Run had returned. Run now also reports a termbox initialization failure instead of returning nil. Co-Authored-By: Claude Opus 5 --- admin.go | 75 +++++++---- admin_test.go | 14 ++ barnard.go | 220 ++++++++++++++++++++++++------- client.go | 165 ++++++++++++++--------- client_notification_test.go | 244 +++++++++++++++++++++++++++++++++++ connection_resource_test.go | 13 ++ recording_control.go | 11 +- ui.go | 168 ++++++++++++++++-------- ui_tree.go | 168 ++++++++++++++---------- ui_tree_snapshot_test.go | 16 +++ ui_tree_test.go | 22 ++++ uiterm/ui.go | 68 ++++++++-- uiterm/ui_regression_test.go | 45 +++++++ 13 files changed, 957 insertions(+), 272 deletions(-) create mode 100644 connection_resource_test.go create mode 100644 ui_tree_snapshot_test.go create mode 100644 ui_tree_test.go create mode 100644 uiterm/ui_regression_test.go diff --git a/admin.go b/admin.go index 2c47ec7..52d0500 100644 --- a/admin.go +++ b/admin.go @@ -65,7 +65,7 @@ func (b *Barnard) OpenAdminMenu() { return } b.adminReturnItem = b.UiTree.ActiveItem() - b.adminTargetUser = b.selectedUser + b.adminTargetUser = b.selectedUserValue() b.adminTargetChan = b.Client.Self.Channel if b.Ui.Active() == uiViewTree { switch item := b.UiTree.ActiveItem().(type) { @@ -83,7 +83,9 @@ func (b *Barnard) OpenAdminMenu() { if b.adminTargetChan != nil { b.adminTargetChan.RequestPermission() } - if root := b.Client.Channels[0]; root != nil && root != b.adminTargetChan { + var root *gumble.Channel + b.Client.Do(func() { root = b.Client.Channels[0] }) + if root != nil && root != b.adminTargetChan { root.RequestPermission() } b.UiAdmin.Rebuild() @@ -490,11 +492,21 @@ func (b *Barnard) adminACLItems() []uiterm.TreeItem { } func (b *Barnard) adminContextActionItems() []uiterm.TreeItem { - if b.Client == nil || len(b.Client.ContextActions) == 0 { + if b.Client == nil { + return []uiterm.TreeItem{adminItem{label: "No context actions available"}} + } + var actions []*gumble.ContextAction + b.Client.Do(func() { + actions = make([]*gumble.ContextAction, 0, len(b.Client.ContextActions)) + for _, action := range b.Client.ContextActions { + actions = append(actions, action) + } + }) + if len(actions) == 0 { return []uiterm.TreeItem{adminItem{label: "No context actions available"}} } items := []uiterm.TreeItem{} - for _, action := range b.Client.ContextActions { + for _, action := range actions { ca := action label := ca.Label if label == "" { @@ -854,7 +866,8 @@ func (b *Barnard) executeContextCommand(fields []string) { b.AddOutputLine("Admin: usage /admin context [server|user|channel] [target]") return } - action := b.Client.ContextActions[fields[1]] + var action *gumble.ContextAction + b.Client.Do(func() { action = b.Client.ContextActions[fields[1]] }) if action == nil { b.AddOutputLine("Admin: context action not found") return @@ -993,39 +1006,47 @@ func (b *Barnard) findOrCreateACLRule(subjectType, subject string) *gumble.ACLRu } } -func (b *Barnard) findUser(token string) *gumble.User { +func (b *Barnard) findUser(token string) (found *gumble.User) { if b.Client == nil { return nil } - if session, err := strconv.ParseUint(token, 10, 32); err == nil { - if user := b.Client.Users[uint32(session)]; user != nil { - return user + b.Client.Do(func() { + if session, err := strconv.ParseUint(token, 10, 32); err == nil { + found = b.Client.Users[uint32(session)] + if found != nil { + return + } } - } - for _, user := range b.Client.Users { - if strings.EqualFold(user.Name, token) { - return user + for _, user := range b.Client.Users { + if strings.EqualFold(user.Name, token) { + found = user + return + } } - } - return nil + }) + return found } -func (b *Barnard) findChannel(token string) *gumble.Channel { +func (b *Barnard) findChannel(token string) (found *gumble.Channel) { if b.Client == nil { return nil } token = strings.TrimSpace(token) - if id, err := strconv.ParseUint(token, 10, 32); err == nil { - if channel := b.Client.Channels[uint32(id)]; channel != nil { - return channel + b.Client.Do(func() { + if id, err := strconv.ParseUint(token, 10, 32); err == nil { + found = b.Client.Channels[uint32(id)] + if found != nil { + return + } } - } - for _, channel := range b.Client.Channels { - if strings.EqualFold(channel.Name, token) { - return channel + for _, channel := range b.Client.Channels { + if strings.EqualFold(channel.Name, token) { + found = channel + return + } } - } - return nil + }) + return found } func (b *Barnard) findRegisteredUser(token string) *gumble.RegisteredUser { @@ -1048,7 +1069,9 @@ func (b *Barnard) adminCanRoot(permission gumble.Permission) bool { if b.Client == nil { return true } - return b.adminCanChannel(b.Client.Channels[0], permission) + var root *gumble.Channel + b.Client.Do(func() { root = b.Client.Channels[0] }) + return b.adminCanChannel(root, permission) } func (b *Barnard) adminCanChannel(channel *gumble.Channel, permission gumble.Permission) bool { diff --git a/admin_test.go b/admin_test.go index 8271173..7e1c169 100644 --- a/admin_test.go +++ b/admin_test.go @@ -7,6 +7,20 @@ import ( "git.stormux.org/storm/barnard/uiterm" ) +// Regression: admin lookup helpers read Client.Users and Client.Channels while +// TCP handlers could mutate those maps. +func TestAdminLookupUsesClientSnapshot(t *testing.T) { + user := &gumble.User{Session: 7, Name: "Guest"} + channel := &gumble.Channel{ID: 4, Name: "Room"} + b := &Barnard{Client: &gumble.Client{Users: gumble.Users{7: user}, Channels: gumble.Channels{4: channel}}} + if b.findUser("guest") != user || b.findUser("7") != user { + t.Fatal("user lookup failed") + } + if b.findChannel("room") != channel || b.findChannel("4") != channel { + t.Fatal("channel lookup failed") + } +} + func TestParseToggleState(t *testing.T) { tests := []struct { name string diff --git a/barnard.go b/barnard.go index 04e1e02..43688db 100644 --- a/barnard.go +++ b/barnard.go @@ -14,8 +14,12 @@ import ( ) type TreeItem struct { - User *gumble.User - Channel *gumble.Channel + User *gumble.User + Channel *gumble.Channel + display string + userSession uint32 + channelID uint32 + snapshot bool } type Barnard struct { @@ -27,25 +31,28 @@ type Barnard struct { Address string TLSConfig tls.Config - Stream *gumbleopenal.Stream - Tx bool - AutoTransmit bool // auto-start transmission on connect - Connected bool + Stream *gumbleopenal.Stream + connectionMutex sync.RWMutex + Tx bool + AutoTransmit bool // auto-start transmission on connect + Connected bool + stateMutex sync.RWMutex - Ui *uiterm.Ui - UiOutput uiterm.Textview - UiInput uiterm.Textbox - UiStatus uiterm.Label - UiTree uiterm.Tree - UiAdmin uiterm.Tree - UiInputStatus uiterm.Label - SelectedChannel *gumble.Channel - selectedUser *gumble.User - adminTargetUser *gumble.User - adminTargetChan *gumble.Channel - adminReturnItem uiterm.TreeItem - statusText string - statusNotice bool + Ui *uiterm.Ui + UiOutput uiterm.Textview + UiInput uiterm.Textbox + UiStatus uiterm.Label + UiTree uiterm.Tree + UiAdmin uiterm.Tree + UiInputStatus uiterm.Label + SelectedChannel *gumble.Channel + selectedUser *gumble.User + selectedUserMutex sync.RWMutex + adminTargetUser *gumble.User + adminTargetChan *gumble.Channel + adminReturnItem uiterm.TreeItem + statusText string + statusNotice bool notifyChannel chan []string @@ -53,8 +60,9 @@ type Barnard struct { exitMessage string // Added for channel muting - MutedChannels map[uint32]bool - userChannels map[uint32]*gumble.Channel + MutedChannels map[uint32]bool + MutedChannelsMutex sync.RWMutex + userChannels map[uint32]*gumble.Channel // Added for noise suppression NoiseSuppressor *noise.Suppressor @@ -80,6 +88,30 @@ type Barnard struct { adminBanList gumble.BanList adminUserList gumble.RegisteredUsers adminACL *gumble.ACL + + reconnectStop chan struct{} + reconnectStopOnce sync.Once +} + +// cleanupConnectionAudio releases connection-owned audio resources before a +// reconnect replaces them. It is intentionally idempotent for repeated +// disconnect notifications. +func (b *Barnard) cleanupConnectionAudio() { + // Connection audio operations that use both resources take FileStreamMutex + // before connectionMutex, so cleanup follows that order as well. + b.FileStreamMutex.Lock() + if b.FileStream != nil { + _ = b.FileStream.Stop() + b.FileStream = nil + } + b.FileStreamMutex.Unlock() + b.connectionMutex.Lock() + if b.Stream != nil { + stream := b.Stream + b.Stream = nil + stream.Destroy() + } + b.connectionMutex.Unlock() } func (b *Barnard) cleanupToneTestAudio() { @@ -93,12 +125,113 @@ func (b *Barnard) cleanupToneTestAudio() { } } +func (b *Barnard) updateUserGain(user *gumble.User) { + b.withStream(func(stream *gumbleopenal.Stream) { + stream.UpdateUserGain(user) + }) +} + +// withStream keeps a connection-owned stream alive for the complete operation. +// Reconnect cleanup takes the write lock before destroying or replacing it. +func (b *Barnard) withStream(action func(*gumbleopenal.Stream)) bool { + b.connectionMutex.RLock() + defer b.connectionMutex.RUnlock() + if b.Stream == nil { + return false + } + action(b.Stream) + return true +} + +func (b *Barnard) isChannelMuted(channelID uint32) bool { + b.MutedChannelsMutex.RLock() + defer b.MutedChannelsMutex.RUnlock() + return b.MutedChannels[channelID] +} + +func (b *Barnard) setChannelMuted(channelID uint32, muted bool) { + b.MutedChannelsMutex.Lock() + defer b.MutedChannelsMutex.Unlock() + if b.MutedChannels == nil { + b.MutedChannels = make(map[uint32]bool) + } + if muted { + b.MutedChannels[channelID] = true + } else { + delete(b.MutedChannels, channelID) + } +} + +func (b *Barnard) selectedUserValue() *gumble.User { + b.selectedUserMutex.RLock() + defer b.selectedUserMutex.RUnlock() + return b.selectedUser +} + +func (b *Barnard) setSelectedUserValue(user *gumble.User) { + b.selectedUserMutex.Lock() + b.selectedUser = user + b.selectedUserMutex.Unlock() +} + +func (b *Barnard) isTransmitting() bool { + b.stateMutex.RLock() + defer b.stateMutex.RUnlock() + return b.Tx +} + +func (b *Barnard) setTransmitting(transmitting bool) { + b.stateMutex.Lock() + b.Tx = transmitting + b.stateMutex.Unlock() +} + +func (b *Barnard) isConnected() bool { + b.stateMutex.RLock() + defer b.stateMutex.RUnlock() + return b.Connected +} + +func (b *Barnard) setConnected(connected bool) { + b.stateMutex.Lock() + b.Connected = connected + b.stateMutex.Unlock() +} + +func (b *Barnard) stopReconnects() { + b.reconnectStopOnce.Do(func() { + if b.reconnectStop != nil { + close(b.reconnectStop) + } + }) +} + +func (b *Barnard) reconnectCanceled() bool { + if b.reconnectStop == nil { + return false + } + select { + case <-b.reconnectStop: + return true + default: + return false + } +} + func (b *Barnard) StopTransmission() { - if b.Tx { + if b.isTransmitting() { b.Notify("micdown", "me", "") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus(" Idle ", false) - b.Stream.StopSource() + if b.ToneTest { + // Stop the tone generator. + if b.toneTestStop != nil { + close(b.toneTestStop) + b.toneTestStop = nil + } + } else { + b.withStream(func(stream *gumbleopenal.Stream) { _ = stream.StopSource() }) + } } } @@ -114,7 +247,7 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm b.GotoChat() } if treeItem.User != nil { - if b.selectedUser == treeItem.User { + if b.selectedUserValue() == treeItem.User { b.SetSelectedUser(nil) b.GotoChat() } else { @@ -128,36 +261,29 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm if treeItem.Channel != nil { if key == *b.Hotkeys.MuteToggle { // Determine new channel mute state - channelWillBeMuted := !b.MutedChannels[treeItem.Channel.ID] + channelWillBeMuted := !b.isChannelMuted(treeItem.Channel.ID) // Set all users in channel to the same mute state users := makeUsersArray(treeItem.Channel.Users) for _, u := range users { // Explicitly set user mute state to match channel state - if channelWillBeMuted != u.LocallyMuted() { + if channelWillBeMuted && !u.LocallyMuted() { + if err := b.UserConfig.ToggleMute(u); err != nil { + b.AddOutputLine("Mute: could not save setting: " + err.Error()) + } + } else if !channelWillBeMuted && u.LocallyMuted() { if err := b.UserConfig.ToggleMute(u); err != nil { b.AddOutputLine("Mute: could not save setting: " + err.Error()) } } - if source := u.AudioSource(); source != nil { - if u.LocallyMuted() { - source.SetGain(0) - } else { - source.SetGain(u.Volume()) - } - } + b.updateUserGain(u) } // Update channel mute state - if channelWillBeMuted { - b.MutedChannels[treeItem.Channel.ID] = true - // If this is the current channel, stop transmission - if b.Client.Self.Channel.ID == treeItem.Channel.ID && b.Tx { - b.StopTransmission() - } - } else { - delete(b.MutedChannels, treeItem.Channel.ID) + b.setChannelMuted(treeItem.Channel.ID, channelWillBeMuted) + if channelWillBeMuted && b.Client.Self.Channel.ID == treeItem.Channel.ID && b.isTransmitting() { + b.StopTransmission() } b.RebuildUserChannelTreePreservingSelection() @@ -180,13 +306,7 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm if err := b.UserConfig.ToggleMute(treeItem.User); err != nil { b.AddOutputLine("Mute: could not save setting: " + err.Error()) } - if source := treeItem.User.AudioSource(); source != nil { - if treeItem.User.LocallyMuted() { - source.SetGain(0) - } else { - source.SetGain(treeItem.User.Volume()) - } - } + b.updateUserGain(treeItem.User) b.RebuildUserChannelTreePreservingSelection() b.Ui.Refresh() } diff --git a/client.go b/client.go index a5f8ac8..3d4e7a4 100644 --- a/client.go +++ b/client.go @@ -14,6 +14,7 @@ import ( ) func (b *Barnard) start() { + b.reconnectStop = make(chan struct{}) b.Config.Attach(gumbleutil.AutoBitrate) b.Config.Attach(b) b.Config.Address = b.Address @@ -46,7 +47,7 @@ func (b *Barnard) exitWithError(err error) { func (b *Barnard) connect(reconnect bool) bool { var err error - _, err = gumble.DialWithDialer(new(net.Dialer), b.Config, &b.TLSConfig) + _, err = gumble.DialWithDialer(&net.Dialer{Timeout: 15 * time.Second}, b.Config, &b.TLSConfig) if err != nil { if reconnect { b.Log(err.Error()) @@ -70,11 +71,11 @@ func (b *Barnard) connect(reconnect bool) bool { b.toneTestSaver = saver b.toneTestSaverDetach = b.Client.Config.AttachAudio(saver) - b.Connected = true + b.setConnected(true) if b.toneTestAutoTransmit() { b.toneTestStop = make(chan struct{}) go StartToneGenerator(b.Client, b.toneTestStop) - b.Tx = true + b.setTransmitting(true) b.UpdateGeneralStatus(" Tx ", true) b.AddOutputLine("Tone test transmission started") } @@ -86,11 +87,17 @@ func (b *Barnard) connect(reconnect bool) bool { b.exitWithError(err) return false } - b.Stream = stream - b.Stream.SetMicVolume(b.UserConfig.GetMicVolume(), false) - b.Stream.AttachStream(b.Client) - b.Stream.SetNoiseProcessor(b.NoiseSuppressor) - b.Stream.SetAGCEnabled(b.UserConfig.GetAGCEnabled()) + stream.SetMicVolume(b.UserConfig.GetMicVolume(), false) + stream.AttachStream(b.Client) + stream.SetNoiseProcessor(b.NoiseSuppressor) + stream.SetAGCEnabled(b.UserConfig.GetAGCEnabled()) + stream.SetErrorFunc(func(err error) { + if err != nil { + b.AddOutputLine(fmt.Sprintf("Microphone: %s", err.Error())) + } else { + b.AddOutputLine("Microphone: recovered") + } + }) // Initialize stereo encoder for file playback b.Client.SetStereoEncoder(opus.NewStereoEncoder()) @@ -103,10 +110,13 @@ func (b *Barnard) connect(reconnect bool) bool { b.Client.DisableStereoEncoder() b.AddOutputLine(fmt.Sprintf("File playback: %s", err.Error())) }) - b.Stream.SetFilePlayer(b.FileStream) + stream.SetFilePlayer(b.FileStream) b.FileStreamMutex.Unlock() + b.connectionMutex.Lock() + b.Stream = stream + b.connectionMutex.Unlock() - b.Connected = true + b.setConnected(true) // Dial delivers OnConnect before connect creates the OpenAL stream, so // start auto-transmit here as well for initial connections and reconnects. b.startAutoTransmit() @@ -117,18 +127,29 @@ func (b *Barnard) OnConnect(e *gumble.ConnectEvent) { b.Client = e.Client // Reset muted channels state on connect + b.MutedChannelsMutex.Lock() b.MutedChannels = make(map[uint32]bool) + b.MutedChannelsMutex.Unlock() b.userChannels = make(map[uint32]*gumble.Channel) b.RecordingMutex.Lock() b.recordingAllowed = nil b.recordingStarting = false b.RecordingMutex.Unlock() - b.Ui.SetActive(uiViewInput) - b.UiTree.Rebuild() - b.Ui.Refresh() + b.postUI(func() { + b.Ui.SetActive(uiViewInput) + b.UiTree.Rebuild() + b.Ui.Refresh() + }) - for _, u := range b.Client.Users { + var users []*gumble.User + b.Client.Do(func() { + users = make([]*gumble.User, 0, len(b.Client.Users)) + for _, u := range b.Client.Users { + users = append(users, u) + } + }) + for _, u := range users { b.UserConfig.UpdateUser(u) b.rememberUserChannel(u) } @@ -143,22 +164,26 @@ func (b *Barnard) OnConnect(e *gumble.ConnectEvent) { if wmsg != "" { b.AddOutputLine(fmt.Sprintf("Welcome message: %s", wmsg)) } - b.Ui.Refresh() b.startAutoTransmit() } func (b *Barnard) startAutoTransmit() { - if !b.AutoTransmit || b.Tx || b.Stream == nil { + if !b.AutoTransmit || b.isTransmitting() { return } - if err := b.Stream.StartSource(b.UserConfig.GetInputDevice()); err != nil { - b.AddOutputLine(fmt.Sprintf("auto-transmit failed: %s", err.Error())) + started := b.withStream(func(stream *gumbleopenal.Stream) { + if err := stream.StartSource(b.UserConfig.GetInputDevice()); err != nil { + b.AddOutputLine(fmt.Sprintf("auto-transmit failed: %s", err.Error())) + return + } + b.setTransmitting(true) + b.UpdateGeneralStatus(" AutoTx ", true) + b.AddOutputLine("Auto-transmit started") + }) + if !started { return } - b.Tx = true - b.UpdateGeneralStatus(" AutoTx ", true) - b.AddOutputLine("Auto-transmit started") } func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { @@ -175,6 +200,7 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { reason = e.String } b.stopRecordingForDisconnect() + b.cleanupConnectionAudio() // Tone test cleanup if b.ToneTest { @@ -191,20 +217,25 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { } else { b.AddOutputLine("Disconnected: " + reason) } - b.Tx = false - b.Connected = false - b.UiTree.Rebuild() - b.Ui.Refresh() + b.setTransmitting(false) + b.setConnected(false) + b.postUI(func() { + b.UiTree.Rebuild() + b.Ui.Refresh() + }) go b.reconnectGoroutine() } func (b *Barnard) reconnectGoroutine() { - for { - res := b.connect(true) - if res == true { - break + for !b.reconnectCanceled() { + if b.connect(true) { + return + } + select { + case <-b.reconnectStop: + return + case <-time.After(15 * time.Second): } - time.Sleep(15 * time.Second) } } @@ -213,15 +244,12 @@ func (b *Barnard) Log(s string) { } func (b *Barnard) OnTextMessage(e *gumble.TextMessageEvent) { - var public = false - for _, c := range e.Channels { - if c.Name == b.Client.Self.Channel.Name { - public = true - break + if b.isPublicTextMessage(e) { + sender := "Server" + if e.Sender != nil { + sender = e.Sender.Name } - } - if public { - b.Notify("msg", e.Sender.Name, e.Message) + b.Notify("msg", sender, e.Message) b.AddOutputMessage(e.Sender, e.Message) } else { var sender string @@ -235,6 +263,28 @@ func (b *Barnard) OnTextMessage(e *gumble.TextMessageEvent) { } } +// isPublicTextMessage reports whether a message targets the current channel, +// either directly or through a recursive channel-tree recipient. +func (b *Barnard) isPublicTextMessage(e *gumble.TextMessageEvent) bool { + if e == nil || b.Client == nil || b.Client.Self == nil || b.Client.Self.Channel == nil { + return false + } + current := b.Client.Self.Channel + for _, channel := range e.Channels { + if sameChannel(channel, current) { + return true + } + } + for _, root := range e.Trees { + for channel := current; channel != nil; channel = channel.Parent { + if sameChannel(channel, root) { + return true + } + } + } + return false +} + func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) { notification, hasNotification := b.userChangeNotification(e) if e.User != nil { @@ -243,20 +293,20 @@ func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) { // Check if user is joining a muted channel if e.Type.Has(gumble.UserChangeConnected) || e.Type.Has(gumble.UserChangeChannel) { // If the channel is muted, ensure the user is muted - if b.MutedChannels[e.User.Channel.ID] { + if b.isChannelMuted(e.User.Channel.ID) { // Only mute if not already muted if !e.User.LocallyMuted() { - b.UserConfig.ToggleMute(e.User) - } - if source := e.User.AudioSource(); source != nil { - source.SetGain(0) + if err := b.UserConfig.ToggleMute(e.User); err != nil { + b.AddOutputLine("Mute: could not save setting: " + err.Error()) + } } + b.updateUserGain(e.User) } } } if e.Type.Has(gumble.UserChangeDisconnected) { - if e.User == b.selectedUser { + if e.User == b.selectedUserValue() { b.SetSelectedUser(nil) } } @@ -281,8 +331,10 @@ func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) { b.AddOutputLine(formatUserStats(e.User)) } b.updateUserChannel(e) - b.RebuildUserChannelTreePreservingSelection() - b.Ui.Refresh() + b.postUI(func() { + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() + }) } type userChangeNotification struct { @@ -382,8 +434,10 @@ func (b *Barnard) OnChannelChange(e *gumble.ChannelChangeEvent) { b.AddOutputLine(fmt.Sprintf("Channel permissions for %s: %s", e.Channel.Name, permissionList(*permission))) } } - b.RebuildUserChannelTreePreservingSelection() - b.Ui.Refresh() + b.postUI(func() { + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() + }) } func formatUserStats(user *gumble.User) string { @@ -461,34 +515,27 @@ func (b *Barnard) OnPermissionDenied(e *gumble.PermissionDeniedEvent) { } func (b *Barnard) OnUserList(e *gumble.UserListEvent) { - b.adminUserList = e.UserList b.AddOutputLine(fmt.Sprintf("Admin: received %d registered users", len(e.UserList))) - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.adminUserList = e.UserList; b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnACL(e *gumble.ACLEvent) { - b.adminACL = e.ACL if e.ACL != nil && e.ACL.Channel != nil { b.AddOutputLine(fmt.Sprintf("Admin: received ACLs for %s", e.ACL.Channel.Name)) } - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.adminACL = e.ACL; b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnBanList(e *gumble.BanListEvent) { - b.adminBanList = e.BanList b.AddOutputLine(fmt.Sprintf("Admin: received %d bans", len(e.BanList))) - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.adminBanList = e.BanList; b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnContextActionChange(e *gumble.ContextActionChangeEvent) { if e.ContextAction != nil { b.AddOutputLine(fmt.Sprintf("Admin: context action updated: %s", e.ContextAction.Name)) } - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnServerConfig(e *gumble.ServerConfigEvent) { diff --git a/client_notification_test.go b/client_notification_test.go index 9eb0f74..9cbde63 100644 --- a/client_notification_test.go +++ b/client_notification_test.go @@ -1,11 +1,255 @@ package main import ( + "fmt" + "io" + "strings" + "sync" "testing" + "time" + "unicode/utf8" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" ) +// Regression: HTML escaping left terminal control sequences in server text, +// allowing ANSI/OSC sequences to alter the terminal that rendered it. +// Regression: a capture-device open error left the application alive with no +// usable microphone. These errors are fatal and use the post-TUI stderr path. +func TestFatalAudioOpenError(t *testing.T) { + for _, err := range []error{gumbleopenal.ErrMic, gumbleopenal.ErrInputDevice, gumbleopenal.ErrOutputDevice, fmt.Errorf("wrapped: %w", gumbleopenal.ErrMic)} { + if !fatalAudioOpenError(err) { + t.Fatalf("%v was not fatal", err) + } + } + if fatalAudioOpenError(gumbleopenal.ErrState) { + t.Fatal("state error should remain recoverable") + } +} + +func TestEscRemovesTerminalControlSequences(t *testing.T) { + got := esc("name\x1b]0;spoof\a\x7f\u202e") + if got != "name]0;spoof" { + t.Fatalf("unsafe terminal text %q", got) + } +} + +type testReadCloser struct{ io.Reader } + +func (testReadCloser) Close() error { return nil } + +// Regression: an EOF from the FIFO was ignored and caused an unbounded busy +// loop. The reader must deliver a final command then close its output. +func TestReadFIFOStopsOnEOF(t *testing.T) { + out := make(chan string) + go readFIFO(testReadCloser{strings.NewReader("command\n")}, out) + if got := <-out; got != "command" { + t.Fatalf("got %q", got) + } + if _, ok := <-out; ok { + t.Fatal("FIFO output remained open after EOF") + } +} + +// 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 TestAudioIntervalDuration(t *testing.T) { + for _, milliseconds := range []int{10, 20, 40, 60} { + got, err := audioIntervalDuration(milliseconds) + if err != nil { + t.Errorf("audioIntervalDuration(%d): %v", milliseconds, err) + continue + } + if got != time.Duration(milliseconds)*time.Millisecond { + t.Errorf("audioIntervalDuration(%d) = %v", milliseconds, got) + } + } + if _, err := audioIntervalDuration(30); err == nil { + t.Fatal("audioIntervalDuration accepted unsupported duration") + } +} + +func TestJitterBufferDuration(t *testing.T) { + for _, milliseconds := range []int{0, 20, 40, 60} { + got, err := jitterBufferDuration(milliseconds) + if err != nil { + t.Errorf("jitterBufferDuration(%d): %v", milliseconds, err) + continue + } + if got != time.Duration(milliseconds)*time.Millisecond { + t.Errorf("jitterBufferDuration(%d) = %v", milliseconds, got) + } + } + if _, err := jitterBufferDuration(10); err == nil { + t.Fatal("jitterBufferDuration accepted unsupported duration") + } +} + +func TestServerAddressDefaultsPortWithoutBreakingIPv6(t *testing.T) { + for input, want := range map[string]string{ + "server": "server:64738", + "server:64739": "server:64739", + "::1": "[::1]:64738", + "[2001:db8::1]": "[2001:db8::1]:64738", + "[2001:db8::1]:9": "[2001:db8::1]:9", + } { + if got := serverAddress(input); got != want { + t.Errorf("serverAddress(%q) = %q, want %q", input, got, want) + } + } +} + +func TestConcurrentConnectionStateAccess(t *testing.T) { + b := &Barnard{} + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(value bool) { + defer wg.Done() + b.setConnected(value) + b.setTransmitting(value) + _ = b.isConnected() + _ = b.isTransmitting() + }(i%2 == 0) + } + wg.Wait() +} + +func TestConcurrentSelectedUserAccess(t *testing.T) { + b := &Barnard{} + user := &gumble.User{Session: 1} + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(user *gumble.User) { + defer wg.Done() + b.setSelectedUserValue(user) + _ = b.selectedUserValue() + }(user) + } + wg.Wait() +} + +func TestConcurrentMutedChannelAccess(t *testing.T) { + b := &Barnard{MutedChannels: make(map[uint32]bool)} + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + b.setChannelMuted(uint32(i%3), i%2 == 0) + _ = b.isChannelMuted(uint32((i + 1) % 3)) + }(i) + } + wg.Wait() +} + +func TestPublicTextMessageTargetsChannelIDsAndTrees(t *testing.T) { + root := &gumble.Channel{ID: 1, Name: "Root"} + current := &gumble.Channel{ID: 2, Name: "Room", Parent: root} + b := &Barnard{Client: &gumble.Client{Self: &gumble.User{Channel: current}}} + + if !b.isPublicTextMessage(&gumble.TextMessageEvent{TextMessage: gumble.TextMessage{Trees: []*gumble.Channel{root}}}) { + t.Fatal("recursive message to an ancestor was not public") + } + other := &gumble.Channel{ID: 3, Name: "Room"} + if b.isPublicTextMessage(&gumble.TextMessageEvent{TextMessage: gumble.TextMessage{Channels: []*gumble.Channel{other}}}) { + t.Fatal("message to a different channel with the same name was public") + } +} + +func TestPublicServerMessageDoesNotPanic(t *testing.T) { + channel := &gumble.Channel{ID: 1, Name: "Current"} + b := &Barnard{ + Client: &gumble.Client{Self: &gumble.User{Channel: channel}}, + notifyChannel: make(chan []string, 1), + } + + b.OnTextMessage(&gumble.TextMessageEvent{ + Client: b.Client, + TextMessage: gumble.TextMessage{ + Channels: []*gumble.Channel{channel}, + Message: "server announcement", + }, + }) + + got := <-b.notifyChannel + want := []string{"msg", "Server", "server announcement"} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] || got[2] != want[2] { + t.Fatalf("notification = %#v, want %#v", got, want) + } +} + +// Regression: reconnect replaced Stream without destroying the old capture +// and renderer resources. Cleanup must be safe for repeated disconnects. +// Regression: user and channel names in the navigation tree bypassed message +// escaping and could still carry terminal control characters. +// Regression: status truncation used byte indexes and could create invalid +// UTF-8 when a non-ASCII user or channel name exceeded the display limit. +func TestTruncateInputStatusPreservesUTF8(t *testing.T) { + got := truncateInputStatus(strings.Repeat("é", 21)) + if !utf8.ValidString(got) || utf8.RuneCountInString(got) != 21 { + t.Fatalf("invalid truncation %q", got) + } +} + +func TestTreeItemSanitizesServerNames(t *testing.T) { + item := TreeItem{Channel: &gumble.Channel{Name: "\x1b[2Jroom"}} + if got := item.String(); got != "#[2Jroom" { + t.Fatalf("got %q", got) + } +} + +func TestCleanupConnectionAudioIsIdempotent(t *testing.T) { + b := &Barnard{} + b.cleanupConnectionAudio() + b.cleanupConnectionAudio() +} + +// Tone test mode intentionally does not create an OpenAL stream. Tree +// controls must therefore keep local mute state without trying to update one. +func TestReconnectCancellationIsSafeBeforeStartup(t *testing.T) { + b := &Barnard{} + b.stopReconnects() + if b.reconnectCanceled() { + t.Fatal("nil reconnect channel should not report cancellation") + } +} + +func TestReconnectCancellationStopsWaiters(t *testing.T) { + b := &Barnard{reconnectStop: make(chan struct{})} + b.stopReconnects() + if !b.reconnectCanceled() { + t.Fatal("expected reconnect cancellation") + } + b.stopReconnects() // repeated shutdown must not panic +} + +func TestUpdateUserGainAllowsToneTestWithoutStream(t *testing.T) { + (&Barnard{ToneTest: true}).updateUserGain(&gumble.User{}) +} + +func TestToneTestRejectsFilePlayback(t *testing.T) { + (&Barnard{ToneTest: true, Connected: true}).CommandPlayFile(nil, "https://example.invalid/audio") +} + func TestUserChangeNotification(t *testing.T) { current := &gumble.Channel{ID: 1, Name: "Current"} other := &gumble.Channel{ID: 2, Name: "Other"} diff --git a/connection_resource_test.go b/connection_resource_test.go new file mode 100644 index 0000000..5415eae --- /dev/null +++ b/connection_resource_test.go @@ -0,0 +1,13 @@ +package main + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumbleopenal" +) + +func TestWithStreamHandlesAbsentConnectionResource(t *testing.T) { + if (&Barnard{}).withStream(func(*gumbleopenal.Stream) {}) { + t.Fatal("nil connection stream was treated as available") + } +} diff --git a/recording_control.go b/recording_control.go index 6e20f16..614effc 100644 --- a/recording_control.go +++ b/recording_control.go @@ -6,6 +6,7 @@ import ( "time" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" "git.stormux.org/storm/barnard/recording" "git.stormux.org/storm/barnard/uiterm" ) @@ -179,10 +180,10 @@ func (b *Barnard) finishRecordingStart() { } b.Recorder = recorder b.recordingStarting = false + // Recorder operations take RecordingMutex before connectionMutex. This + // prevents disconnect cleanup from destroying a stream during attachment. + b.withStream(func(stream *gumbleopenal.Stream) { stream.SetRecorder(recorder) }) b.RecordingMutex.Unlock() - if b.Stream != nil { - b.Stream.SetRecorder(recorder) - } b.AddOutputLine(fmt.Sprintf("Recording started: %s", recorder.Path())) b.Notify("recordstart", "me", recorder.Path()) b.renderGeneralStatus() @@ -205,9 +206,7 @@ func (b *Barnard) detachRecorder() (*recording.Recorder, string, bool) { } b.Recorder = nil b.recordingStarting = false - if b.Stream != nil { - b.Stream.SetRecorder(nil) - } + b.withStream(func(stream *gumbleopenal.Stream) { stream.SetRecorder(nil) }) return recorder, path, wasPending } diff --git a/ui.go b/ui.go index e66848e..6d2964e 100644 --- a/ui.go +++ b/ui.go @@ -1,6 +1,7 @@ package main import ( + "errors" "fmt" "os" "strings" @@ -8,6 +9,7 @@ import ( "unicode" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" "git.stormux.org/storm/barnard/uiterm" "github.com/kennygrant/sanitize" "github.com/nsf/termbox-go" @@ -37,6 +39,14 @@ func esc(str string) string { return sanitize.HTML(clean) } +// postUI is the only path network and audio callbacks use to touch terminal +// widgets. Work is dropped during shutdown or queue overload. +func (b *Barnard) postUI(fn func()) { + if b.Ui != nil { + b.Ui.Post(fn) + } +} + func (b *Barnard) Notify(event string, who string, what string) { // 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. @@ -47,7 +57,7 @@ func (b *Barnard) Notify(event string, who string, what string) { } func (b *Barnard) SetSelectedUser(user *gumble.User) { - b.selectedUser = user + b.setSelectedUserValue(user) if user == nil { if len(b.UiInput.Text) > 0 { } @@ -63,12 +73,21 @@ func (b *Barnard) GetInputStatus() string { func (b *Barnard) UpdateInputStatus(status string) { status = truncateInputStatus(status) - b.UiInputStatus.Text = status - b.RebuildUserChannelTreePreservingSelection() - b.Ui.Refresh() + if b.Ui == nil { + return + } + b.Ui.Post(func() { + b.UiInputStatus.Text = status + // The initial connection status arrives after Run's first layout. Relayout + // so the prompt has cells to draw before focus is changed. + width, height := termbox.Size() + b.OnUiResize(b.Ui, width, height) + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() + }) } -// truncateInputStatus shortens the prompt without splitting a multi-byte rune. +// truncateInputStatus limits terminal cells without splitting UTF-8 runes. func truncateInputStatus(status string) string { chars := []rune(status) if len(chars) > 20 { @@ -79,7 +98,11 @@ func truncateInputStatus(status string) string { func (b *Barnard) AddOutputLine(line string) { now := time.Now() - b.UiOutput.AddLine(fmt.Sprintf("%s [%02d:%02d:%02d]", line, now.Hour(), now.Minute(), now.Second())) + formatted := fmt.Sprintf("%s [%02d:%02d:%02d]", line, now.Hour(), now.Minute(), now.Second()) + if b.Ui == nil { + return + } + b.Ui.Post(func() { b.UiOutput.AddLine(formatted) }) } func (b *Barnard) AddOutputMessage(sender *gumble.User, message string) { @@ -135,19 +158,26 @@ func (b *Barnard) toggleAGC() bool { if err := b.UserConfig.SetAGCEnabled(enabled); err != nil { b.AddOutputLine("AGC: could not save setting: " + err.Error()) } - if b.Stream != nil { - b.Stream.SetAGCEnabled(enabled) - } + b.withStream(func(stream *gumbleopenal.Stream) { + stream.SetAGCEnabled(enabled) + }) return enabled } func (b *Barnard) UpdateGeneralStatus(text string, notice bool) { - b.statusText = text - b.statusNotice = notice - b.renderGeneralStatus() + b.postUI(func() { + b.statusText = text + b.statusNotice = notice + b.renderGeneralStatusNow() + }) } func (b *Barnard) renderGeneralStatus() { + b.postUI(func() { b.renderGeneralStatusNow() }) +} + +// renderGeneralStatusNow must run on the UI-owning goroutine. +func (b *Barnard) renderGeneralStatusNow() { text := b.statusText notice := b.statusNotice if notice { @@ -241,7 +271,7 @@ func (b *Barnard) CommandPlayFile(ui *uiterm.Ui, cmd string) { } } - if !b.Connected { + if !b.isConnected() { b.AddOutputLine("Not connected to server") return } @@ -252,8 +282,12 @@ func (b *Barnard) CommandPlayFile(ui *uiterm.Ui, cmd string) { b.FileStreamMutex.Lock() defer b.FileStreamMutex.Unlock() + if b.FileStream == nil { + b.AddOutputLine("File playback is unavailable while reconnecting") + return + } - if b.FileStream != nil && b.FileStream.IsPlaying() { + if b.FileStream.IsPlaying() { b.AddOutputLine("Already playing a file. Use /stop first.") return } @@ -267,16 +301,23 @@ func (b *Barnard) CommandPlayFile(ui *uiterm.Ui, cmd string) { // Enable stereo encoder for file playback b.Client.EnableStereoEncoder() - // Auto-start transmission if not already transmitting - if !b.Tx { - err := b.Stream.StartSource(b.UserConfig.GetInputDevice()) - if err != nil { - b.AddOutputLine(fmt.Sprintf("Error starting transmission: %s", err.Error())) + // Auto-start transmission if not already transmitting. FileStreamMutex is + // held here, before withStream's connection mutex, matching cleanup. + if !b.isTransmitting() { + var startErr error + started := b.withStream(func(stream *gumbleopenal.Stream) { + startErr = stream.StartSource(b.UserConfig.GetInputDevice()) + }) + if !started { + startErr = errors.New("audio unavailable while reconnecting") + } + if startErr != nil { + b.AddOutputLine(fmt.Sprintf("Error starting transmission: %s", startErr)) b.FileStream.Stop() b.Client.DisableStereoEncoder() return } - b.Tx = true + b.setTransmitting(true) b.UpdateGeneralStatus(" File ", true) } @@ -313,15 +354,15 @@ func (b *Barnard) CommandStopFile(ui *uiterm.Ui, cmd string) { } func (b *Barnard) setTransmit(ui *uiterm.Ui, val int) { - if b.Tx && val == 1 { + if b.isTransmitting() && val == 1 { return } - if b.Tx == false && val == 0 { + if !b.isTransmitting() && val == 0 { return } - if b.Tx { + if b.isTransmitting() { b.Notify("micdown", "me", "") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus(" Idle ", false) if b.ToneTest { if b.toneTestStop != nil { @@ -329,73 +370,96 @@ func (b *Barnard) setTransmit(ui *uiterm.Ui, val int) { b.toneTestStop = nil } } else { - b.Stream.StopSource() + b.withStream(func(stream *gumbleopenal.Stream) { _ = stream.StopSource() }) } - } else if b.Connected == false { + } else if !b.isConnected() { b.Notify("error", "me", "no tx while disconnected") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus("no tx while disconnected", true) - } else if b.MutedChannels[b.Client.Self.Channel.ID] { + } else if b.isChannelMuted(b.Client.Self.Channel.ID) { // Check if current channel is muted b.Notify("error", "me", "cannot transmit in muted channel") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus("cannot transmit in muted channel", true) } else { - b.Tx = true + b.setTransmitting(true) if b.ToneTest { b.toneTestStop = make(chan struct{}) go StartToneGenerator(b.Client, b.toneTestStop) b.Notify("micup", "me", "") b.UpdateGeneralStatus(" Tx ", true) } else { - err := b.Stream.StartSource(b.UserConfig.GetInputDevice()) - if err != nil { - b.Notify("error", "me", err.Error()) - b.UpdateGeneralStatus(err.Error(), true) - } else { + started := b.withStream(func(stream *gumbleopenal.Stream) { + err := stream.StartSource(b.UserConfig.GetInputDevice()) + if err != nil { + b.setTransmitting(false) + if fatalAudioOpenError(err) { + // A missing capture device cannot recover through normal + // transmission controls; exit so option 1 reports it on stderr. + b.exitWithError(fmt.Errorf("audio device initialization failed: %w", err)) + return + } + b.Notify("error", "me", err.Error()) + b.UpdateGeneralStatus(err.Error(), true) + return + } b.Notify("micup", "me", "") b.UpdateGeneralStatus(" Tx ", true) + }) + if !started { + b.setTransmitting(false) + b.UpdateGeneralStatus("audio unavailable while reconnecting", true) } } } } +func fatalAudioOpenError(err error) bool { + return errors.Is(err, gumbleopenal.ErrMic) || errors.Is(err, gumbleopenal.ErrInputDevice) || errors.Is(err, gumbleopenal.ErrOutputDevice) +} + func (b *Barnard) OnMicVolumeDown(ui *uiterm.Ui, key uiterm.Key) { if b.ToneTest { return } - b.Stream.SetMicVolume(-0.1, true) - b.UserConfig.SetMicVolume(b.Stream.GetMicVolume()) - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Microphone: could not save volume: " + err.Error()) - } + b.withStream(func(stream *gumbleopenal.Stream) { + stream.SetMicVolume(-0.1, true) + b.UserConfig.SetMicVolume(stream.GetMicVolume()) + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Microphone: could not save volume: " + err.Error()) + } + }) } func (b *Barnard) OnMicVolumeUp(ui *uiterm.Ui, key uiterm.Key) { if b.ToneTest { return } - b.Stream.SetMicVolume(0.1, true) - b.UserConfig.SetMicVolume(b.Stream.GetMicVolume()) - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Microphone: could not save volume: " + err.Error()) - } + b.withStream(func(stream *gumbleopenal.Stream) { + stream.SetMicVolume(0.1, true) + b.UserConfig.SetMicVolume(stream.GetMicVolume()) + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Microphone: could not save volume: " + err.Error()) + } + }) } func (b *Barnard) OnQuitPress(ui *uiterm.Ui, key uiterm.Key) { + b.stopReconnects() b.StopRecordingIfActive(true) b.Client.Disconnect() b.Ui.Close() } func (b *Barnard) CommandExit(ui *uiterm.Ui, cmd string) { + b.stopReconnects() b.StopRecordingIfActive(true) b.Client.Disconnect() b.Ui.Close() } func (b *Barnard) CommandStatus(ui *uiterm.Ui, cmd string) { - if b.Tx { + if b.isTransmitting() { b.Notify("status", "me", "transmitting") } else { b.Notify("status", "me", "not transmitting") @@ -485,9 +549,9 @@ func (b *Barnard) OnTextInput(ui *uiterm.Ui, textbox *uiterm.Textbox, text strin // Not a command, send as chat message if b.Client != nil && b.Client.Self != nil { - if b.selectedUser != nil { - b.selectedUser.Send(text) - b.AddOutputPrivateMessage(b.Client.Self, b.selectedUser, text) + if selectedUser := b.selectedUserValue(); selectedUser != nil { + selectedUser.Send(text) + b.AddOutputPrivateMessage(b.Client.Self, selectedUser, text) } else { b.Client.Self.Channel.Send(text, false) b.AddOutputMessage(b.Client.Self, text) @@ -527,8 +591,9 @@ func (b *Barnard) OnUiInitialize(ui *uiterm.Ui) { ui.Add(uiViewInput, &b.UiInput) b.UiInputStatus = uiterm.Label{ - Fg: uiterm.ColorBlack, - Bg: uiterm.ColorWhite, + Text: "[root]", + Fg: uiterm.ColorBlack, + Bg: uiterm.ColorWhite, } ui.Add(uiViewInputStatus, &b.UiInputStatus) @@ -575,6 +640,7 @@ func (b *Barnard) OnUiInitialize(ui *uiterm.Ui) { b.Ui.AddKeyListener(b.OnNoiseSuppressionToggle, b.Hotkeys.NoiseSuppressionToggle) b.Ui.AddKeyListener(b.OnAGCToggle, b.Hotkeys.AGCToggle) b.Ui.AddKeyListener(b.OnRecordingToggle, b.Hotkeys.RecordToggle) + b.Ui.AddKeyListener(b.OnClearPress, b.Hotkeys.ClearOutput) b.Ui.AddKeyListener(b.OnQuitPress, b.Hotkeys.Exit) b.Ui.AddKeyListener(b.OnScrollOutputUp, b.Hotkeys.ScrollUp) b.Ui.AddKeyListener(b.OnScrollOutputDown, b.Hotkeys.ScrollDown) diff --git a/ui_tree.go b/ui_tree.go index ce2e556..04af22e 100644 --- a/ui_tree.go +++ b/ui_tree.go @@ -3,11 +3,15 @@ package main import ( "fmt" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" "git.stormux.org/storm/barnard/uiterm" "sort" ) func (ti TreeItem) String() string { + if ti.display != "" { + return ti.display + } if ti.User != nil { if ti.User.LocallyMuted() { return "[MUTED] " + esc(ti.User.Name) @@ -34,59 +38,61 @@ func (ti TreeItem) TreeItemStyle(fg, bg uiterm.Attribute, active bool) (uiterm.A } func (b *Barnard) changeVolume(users []*gumble.User, change float32) { - for _, u := range users { - au := u.AudioSource() - if au == nil { - continue + changed := b.withStream(func(stream *gumbleopenal.Stream) { + for _, u := range users { + var boost uint16 + var ng float32 + curboost := float32((u.Boost() - 1)) / 10 + ng = u.Volume() + curboost + change + boost = uint16(1) + if ng > 1.0 { + perc := uint16((ng * 10)) - 10 + perc += 1 + boost = perc + ng = 1.0 + } + if ng < 0 { + ng = 0.0 + } + u.SetBoost(boost) + u.SetVolume(ng) + stream.UpdateUserGain(u) + b.UserConfig.UpdateConfig(u) } - var boost uint16 - var cv float32 - var ng float32 - var curboost float32 - curboost = float32((u.Boost() - 1)) / 10 - cv = au.GetGain() + curboost - ng = cv + change - boost = uint16(1) - if ng > 1.0 { - perc := uint16((ng * 10)) - 10 - perc += 1 - boost = perc - ng = 1.0 + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Volume: could not save setting: " + err.Error()) } - if ng < 0 { - ng = 0.0 - } - u.SetBoost(boost) - u.SetVolume(ng) - if !u.LocallyMuted() { - au.SetGain(ng) - } - b.UserConfig.UpdateConfig(u) - } - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Volume: could not save setting: " + err.Error()) + }) + if changed { + b.refreshVolumeDisplay() } } func (b *Barnard) resetVolume(users []*gumble.User) { - for _, u := range users { - au := u.AudioSource() - if au == nil { - continue + changed := b.withStream(func(stream *gumbleopenal.Stream) { + for _, u := range users { + // Reset to original volume (1.0) and boost (1) + u.SetBoost(uint16(1)) + u.SetVolume(1.0) + stream.UpdateUserGain(u) + b.UserConfig.UpdateConfig(u) } - // Reset to original volume (1.0) and boost (1) - u.SetBoost(uint16(1)) - u.SetVolume(1.0) - if !u.LocallyMuted() { - au.SetGain(1.0) + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Volume: could not save setting: " + err.Error()) } - b.UserConfig.UpdateConfig(u) - } - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Volume: could not save setting: " + err.Error()) + }) + if changed { + b.refreshVolumeDisplay() } } +// Tree items render a display string snapshotted at build time, so a volume +// change is only visible after the tree is rebuilt. +func (b *Barnard) refreshVolumeDisplay() { + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() +} + func makeUsersArray(users gumble.Users) []*gumble.User { t := make([]*gumble.User, 0, len(users)) for _, u := range users { @@ -102,15 +108,18 @@ func (b *Barnard) TreeItemBuild(item uiterm.TreeItem) []uiterm.TreeItem { var treeItem TreeItem if ti, ok := item.(TreeItem); !ok { - root := b.Client.Channels[0] + var root *gumble.Channel + b.Client.Do(func() { root = b.Client.Channels[0] }) if root == nil { return nil } - return []uiterm.TreeItem{ - TreeItem{ - Channel: root, - }, - } + var display string + var channelID uint32 + b.Client.Do(func() { + display = "#" + esc(root.Name) + channelID = root.ID + }) + return []uiterm.TreeItem{TreeItem{Channel: root, display: display, channelID: channelID, snapshot: true}} } else { treeItem = ti } @@ -120,39 +129,52 @@ func (b *Barnard) TreeItemBuild(item uiterm.TreeItem) []uiterm.TreeItem { } users := []uiterm.TreeItem{} - ul := []*gumble.User{} - for _, user := range treeItem.Channel.Users { - ul = append(ul, user) - var u = ul[len(ul)-1] - _ = u + type userDisplay struct { + user *gumble.User + display string + name string + session uint32 } + type channelDisplay struct { + channel *gumble.Channel + name string + id uint32 + } + ul := []userDisplay{} + cl := []channelDisplay{} + // TCP handlers mutate both maps; snapshot them while Client.Do holds its + // read lock, then sort/render outside the protocol critical section. + b.Client.Do(func() { + for _, user := range treeItem.Channel.Users { + boostPercent := float32(user.Boost()-1) * 10 + totalVolume := user.Volume()*100 + boostPercent + display := fmt.Sprintf("%s [%.0f%%]", esc(user.Name), totalVolume) + if user.LocallyMuted() { + display = "[MUTED] " + display + } + ul = append(ul, userDisplay{user: user, name: user.Name, session: user.Session, display: display}) + } + for _, subchannel := range treeItem.Channel.Children { + cl = append(cl, channelDisplay{channel: subchannel, name: subchannel.Name, id: subchannel.ID}) + } + }) sort.Slice(ul, func(i, j int) bool { - return ul[i].Name < ul[j].Name + return ul[i].name < ul[j].name }) for _, user := range ul { - users = append(users, TreeItem{ - User: user, - }) + users = append(users, TreeItem{User: user.user, display: user.display, userSession: user.session, snapshot: true}) } channels := []uiterm.TreeItem{} - cl := []*gumble.Channel{} - for _, subchannel := range treeItem.Channel.Children { - cl = append(cl, subchannel) - } sort.Slice(cl, func(i, j int) bool { - return cl[i].Name < cl[j].Name + return cl[i].name < cl[j].name }) for _, subchannel := range cl { - displayName := subchannel.Name - if b.MutedChannels[subchannel.ID] { - displayName = "[MUTED] #" + displayName - } else { - displayName = "#" + displayName + displayName := "#" + esc(subchannel.name) + if b.isChannelMuted(subchannel.id) { + displayName = "[MUTED] " + displayName } - channels = append(channels, TreeItem{ - Channel: subchannel, - }) + channels = append(channels, TreeItem{Channel: subchannel.channel, display: displayName, channelID: subchannel.id, snapshot: true}) } return append(users, channels...) @@ -172,9 +194,15 @@ func sameUserChannelTreeItem(previous, current uiterm.TreeItem) bool { return false } if prev.User != nil && cur.User != nil { + if prev.snapshot && cur.snapshot { + return prev.userSession == cur.userSession + } return prev.User.Session == cur.User.Session } if prev.Channel != nil && cur.Channel != nil { + if prev.snapshot && cur.snapshot { + return prev.channelID == cur.channelID + } return prev.Channel.ID == cur.Channel.ID } return false diff --git a/ui_tree_snapshot_test.go b/ui_tree_snapshot_test.go new file mode 100644 index 0000000..219c2ad --- /dev/null +++ b/ui_tree_snapshot_test.go @@ -0,0 +1,16 @@ +package main + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumble" +) + +func TestTreeItemUsesCapturedDisplaySnapshot(t *testing.T) { + user := &gumble.User{Name: "before"} + item := TreeItem{User: user, display: "before [100%]"} + user.Name = "after" + if got := item.String(); got != "before [100%]" { + t.Fatalf("tree display read mutable user state: %q", got) + } +} diff --git a/ui_tree_test.go b/ui_tree_test.go new file mode 100644 index 0000000..69db060 --- /dev/null +++ b/ui_tree_test.go @@ -0,0 +1,22 @@ +package main + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumble" +) + +// Regression: rebuilding the channel tree ranged protocol-owned maps without +// Client.Do while TCP handlers could add or remove users/channels. +func TestTreeItemBuildReadsMapsUnderClientSnapshot(t *testing.T) { + root := &gumble.Channel{ID: 0, Users: gumble.Users{}, Children: gumble.Channels{}} + user := &gumble.User{Session: 1, Name: "user"} + child := &gumble.Channel{ID: 2, Name: "child", Users: gumble.Users{}, Children: gumble.Channels{}} + root.Users[user.Session] = user + root.Children[child.ID] = child + b := &Barnard{Client: &gumble.Client{Channels: gumble.Channels{0: root}}, MutedChannels: map[uint32]bool{}} + items := b.TreeItemBuild(TreeItem{Channel: root}) + if len(items) != 2 { + t.Fatalf("got %d items", len(items)) + } +} diff --git a/uiterm/ui.go b/uiterm/ui.go index d34c74c..c6041b9 100644 --- a/uiterm/ui.go +++ b/uiterm/ui.go @@ -3,7 +3,9 @@ package uiterm import ( "errors" "strings" + "sync" "sync/atomic" + "time" "github.com/nsf/termbox-go" ) @@ -20,8 +22,10 @@ type UiManager interface { type Ui struct { Fg, Bg Attribute - close chan bool - manager UiManager + close chan struct{} + closeOnce sync.Once + events chan func() + manager UiManager drawCount int32 elements map[string]*uiElement @@ -39,7 +43,8 @@ type uiElement struct { func New(manager UiManager) *Ui { ui := &Ui{ - close: make(chan bool, 10), + close: make(chan struct{}), + events: make(chan func(), 256), elements: make(map[string]*uiElement), manager: manager, keyListeners: make(map[Key][]KeyListener), @@ -48,9 +53,24 @@ func New(manager UiManager) *Ui { return ui } +// Close is safe to call repeatedly and never blocks a caller. func (ui *Ui) Close() { - if termbox.IsInit { - ui.close <- true + ui.closeOnce.Do(func() { close(ui.close) }) +} + +// Post schedules UI work on Run's owning goroutine. It is deliberately +// bounded: network callbacks must not block behind slow terminal rendering. +func (ui *Ui) Post(fn func()) bool { + if fn == nil { + return true + } + select { + case <-ui.close: + return false + case ui.events <- fn: + return true + default: + return false } } @@ -97,15 +117,37 @@ func (ui *Ui) Run(cmds chan string) error { return nil } if err := termbox.Init(); err != nil { - return nil + return err } - defer termbox.Close() termbox.SetInputMode(termbox.InputAlt) - events := make(chan termbox.Event) + // Closing termbox wakes PollEvent. Keep delivery cancellable so the polling + // goroutine cannot become stranded trying to send after Run returns. + events := make(chan termbox.Event, 1) + pollDone := make(chan struct{}) go func() { + defer close(pollDone) for { - events <- termbox.PollEvent() + event := termbox.PollEvent() + select { + case <-ui.close: + return + default: + } + select { + case events <- event: + case <-ui.close: + return + } + } + }() + defer func() { + termbox.Close() + // Some termbox backends do not wake PollEvent promptly on Close. A fatal + // startup failure must print its stderr error instead of hanging here. + select { + case <-pollDone: + case <-time.After(100 * time.Millisecond): } }() @@ -119,7 +161,13 @@ func (ui *Ui) Run(cmds chan string) error { select { case <-ui.close: return nil - case cmd := <-cmds: + case fn := <-ui.events: + fn() + case cmd, ok := <-cmds: + if !ok { + cmds = nil + continue + } ui.onCommandEvent(cmd) case event := <-events: switch event.Type { diff --git a/uiterm/ui_regression_test.go b/uiterm/ui_regression_test.go new file mode 100644 index 0000000..4598959 --- /dev/null +++ b/uiterm/ui_regression_test.go @@ -0,0 +1,45 @@ +package uiterm + +import "testing" + +// Regression: Close sent to a bounded channel and could block or enqueue +// duplicate shutdowns when called more than once. +// Regression: network callbacks modified terminal state directly. Post gives +// them a bounded handoff to the UI-owning Run goroutine instead of blocking. +func TestPostIsBoundedAndRejectsClosedUI(t *testing.T) { + ui := New(nil) + for i := 0; i < cap(ui.events); i++ { + if !ui.Post(func() {}) { + t.Fatal("queue filled too early") + } + } + if ui.Post(func() {}) { + t.Fatal("Post accepted work past queue capacity") + } + ui.Close() + if ui.Post(func() {}) { + t.Fatal("Post accepted work after close") + } +} + +func TestCloseIsNonblockingAndIdempotent(t *testing.T) { + ui := New(nil) + ui.Close() + ui.Close() + select { + case <-ui.close: + default: + t.Fatal("Close did not signal shutdown") + } +} + +func TestSafeRuneRemovesTerminalControlCharacters(t *testing.T) { + for _, r := range []rune{'\x1b', '\x7f', '\u202e'} { + if got := safeRune(r); got != ' ' { + t.Errorf("safeRune(%U) = %U, want space", r, got) + } + } + if got := safeRune('A'); got != 'A' { + t.Fatalf("safeRune altered printable text: %U", got) + } +}