From 554a84098208c0e2d0182551b88fd04fc2a2d475 Mon Sep 17 00:00:00 2001 From: "5684185+vsariola@users.noreply.github.com" <5684185+vsariola@users.noreply.github.com> Date: Wed, 30 Apr 2025 22:42:35 +0300 Subject: [PATCH] refactor(tracker): new closing mechanism logic --- cmd/sointu-track/main.go | 4 +- cmd/sointu-vsti/main.go | 18 ++++-- tracker/action.go | 8 ++- tracker/broker.go | 48 +++++++++++++-- tracker/detector.go | 107 +++++++++++++++++---------------- tracker/gioui/keyevent.go | 2 +- tracker/gioui/songpanel.go | 2 +- tracker/gioui/tracker.go | 118 +++++++++++++++++-------------------- tracker/model_test.go | 2 +- 9 files changed, 172 insertions(+), 137 deletions(-) diff --git a/cmd/sointu-track/main.go b/cmd/sointu-track/main.go index 794a6fa..82a237b 100644 --- a/cmd/sointu-track/main.go +++ b/cmd/sointu-track/main.go @@ -8,6 +8,7 @@ import ( "path/filepath" "runtime" "runtime/pprof" + "time" "gioui.org/app" "github.com/vsariola/sointu" @@ -71,7 +72,8 @@ func main() { go func() { trackerUi.Main() audioCloser.Close() - detector.Close() + tracker.TrySend(broker.CloseDetector, struct{}{}) + tracker.TimeoutReceive(broker.FinishedDetector, 3*time.Second) if *cpuprofile != "" { pprof.StopCPUProfile() f.Close() diff --git a/cmd/sointu-vsti/main.go b/cmd/sointu-vsti/main.go index 3f44903..b713a34 100644 --- a/cmd/sointu-vsti/main.go +++ b/cmd/sointu-vsti/main.go @@ -9,6 +9,7 @@ import ( "math" "os" "path/filepath" + "time" "github.com/vsariola/sointu" "github.com/vsariola/sointu/cmd" @@ -134,17 +135,22 @@ func init() { } }, CloseFunc: func() { - broker.ToModel <- tracker.MsgToModel{Data: func() { t.ForceQuit().Do() }} - t.WaitQuitted() - detector.Close() + tracker.TrySend(broker.CloseDetector, struct{}{}) + tracker.TrySend(broker.CloseGUI, struct{}{}) + tracker.TimeoutReceive(broker.FinishedDetector, 3*time.Second) + tracker.TimeoutReceive(broker.FinishedGUI, 3*time.Second) }, GetChunkFunc: func(isPreset bool) []byte { retChn := make(chan []byte) - broker.ToModel <- tracker.MsgToModel{Data: func() { retChn <- t.MarshalRecovery() }} - return <-retChn + + if !tracker.TrySend(broker.ToModel, tracker.MsgToModel{Data: func() { retChn <- t.MarshalRecovery() }}) { + return nil + } + ret, _ := tracker.TimeoutReceive(retChn, 5*time.Second) // ret will be nil if timeout or channel closed + return ret }, SetChunkFunc: func(data []byte, isPreset bool) { - broker.ToModel <- tracker.MsgToModel{Data: func() { t.UnmarshalRecovery(data) }} + tracker.TrySend(broker.ToModel, tracker.MsgToModel{Data: func() { t.UnmarshalRecovery(data) }}) }, } diff --git a/tracker/action.go b/tracker/action.go index 9944530..b8af6ec 100644 --- a/tracker/action.go +++ b/tracker/action.go @@ -436,10 +436,12 @@ func (m *Model) OpenSong() Action { }) } -func (m *Model) Quit() Action { +func (m *Model) RequestQuit() Action { return Allow(func() { - m.dialog = QuitChanges - m.completeAction(true) + if !m.quitted { + m.dialog = QuitChanges + m.completeAction(true) + } }) } diff --git a/tracker/broker.go b/tracker/broker.go index 049d37b..acab410 100644 --- a/tracker/broker.go +++ b/tracker/broker.go @@ -2,6 +2,7 @@ package tracker import ( "sync" + "time" "github.com/vsariola/sointu" "github.com/vsariola/sointu/vm" @@ -16,11 +17,33 @@ type ( // return buffers to pass buffers around without allocating new memory every // time. We can later consider making many-to-many types of communication // and more complex routing logic to the Broker if needed. + // + // For closing goroutines, the broker has two channels for each goroutine: + // CloseXXX and FinishedXXX. The CloseXXX channel has a capacity of 1, so + // you can always send a empty message (struct{}{}) to it without blocking. + // If the channel is already full, that means someone else has already + // requested its closure and the goroutine is already closing, so dropping + // the message is fine. Then, FinishedXXX is used to signal that a goroutine + // has succesfully closed and cleaned up. Nothing is ever sent to the + // channel, it is only closed. You can wait until the goroutines is done + // closing with "<- FinishedXXX", which for avoiding deadlocks can be + // combined with a timeout: + // select { + // case <-FinishedXXX: + // case <-time.After(3 * time.Second): + // } + Broker struct { ToModel chan MsgToModel ToPlayer chan any // TODO: consider using a sum type here, for a bit more type safety. See: https://www.jerf.org/iri/post/2917/ ToDetector chan MsgToDetector + CloseDetector chan struct{} + CloseGUI chan struct{} + + FinishedGUI chan struct{} + FinishedDetector chan struct{} + bufferPool sync.Pool } @@ -49,7 +72,6 @@ type ( // which gets executed in the detector goroutine. MsgToDetector struct { Reset bool - Quit bool Data any // TODO: consider using a sum type here, for a bit more type safety. See: https://www.jerf.org/iri/post/2917/ WeightingType WeightingType @@ -61,10 +83,14 @@ type ( func NewBroker() *Broker { return &Broker{ - ToPlayer: make(chan interface{}, 1024), - ToModel: make(chan MsgToModel, 1024), - ToDetector: make(chan MsgToDetector, 1024), - bufferPool: sync.Pool{New: func() interface{} { return &sointu.AudioBuffer{} }}, + ToPlayer: make(chan interface{}, 1024), + ToModel: make(chan MsgToModel, 1024), + ToDetector: make(chan MsgToDetector, 1024), + CloseDetector: make(chan struct{}, 1), + CloseGUI: make(chan struct{}, 1), + FinishedGUI: make(chan struct{}), + FinishedDetector: make(chan struct{}), + bufferPool: sync.Pool{New: func() interface{} { return &sointu.AudioBuffer{} }}, } } @@ -96,3 +122,15 @@ func TrySend[T any](c chan<- T, v T) bool { } return true } + +// TimeoutReceive is a helper function to block until a value is received from a +// channel, or timing out after t. ok will be false if the timeout occurred or +// if the channel is closed. +func TimeoutReceive[T any](c <-chan T, t time.Duration) (v T, ok bool) { + select { + case v, ok = <-c: + return v, ok + case <-time.After(t): + return v, false + } +} diff --git a/tracker/detector.go b/tracker/detector.go index 9af787e..d21ef34 100644 --- a/tracker/detector.go +++ b/tracker/detector.go @@ -12,6 +12,7 @@ type ( broker *Broker loudnessDetector loudnessDetector peakDetector peakDetector + chunkHistory sointu.AudioBuffer } WeightingType int @@ -98,65 +99,63 @@ func NewDetector(b *Broker) *Detector { } func (s *Detector) Run() { - var chunkHistory sointu.AudioBuffer - for msg := range s.broker.ToDetector { - if msg.Reset { - s.loudnessDetector.reset() - s.peakDetector.reset() - } - if msg.Quit { + for { + select { + case <-s.broker.CloseDetector: + close(s.broker.FinishedDetector) return - } - if msg.HasWeightingType { - s.loudnessDetector.weighting = weightings[WeightingType(msg.WeightingType)] - s.loudnessDetector.reset() - } - if msg.HasOversampling { - s.peakDetector.oversampling = msg.Oversampling - s.peakDetector.reset() - } - - switch data := msg.Data.(type) { - case *sointu.AudioBuffer: - buf := *data - for { - var chunk sointu.AudioBuffer - if len(chunkHistory) > 0 && len(chunkHistory) < 4410 { - l := min(len(buf), 4410-len(chunkHistory)) - chunkHistory = append(chunkHistory, buf[:l]...) - if len(chunkHistory) < 4410 { - break - } - chunk = chunkHistory - buf = buf[l:] - } else { - if len(buf) >= 4410 { - chunk = buf[:4410] - buf = buf[4410:] - } else { - chunkHistory = chunkHistory[:0] - chunkHistory = append(chunkHistory, buf...) - break - } - } - TrySend(s.broker.ToModel, MsgToModel{ - HasDetectorResult: true, - DetectorResult: DetectorResult{ - Loudness: s.loudnessDetector.update(chunk), - Peaks: s.peakDetector.update(chunk), - }, - }) - } - s.broker.PutAudioBuffer(data) - case func(): - data() + case msg := <-s.broker.ToDetector: + s.handleMsg(msg) } } } -// Close may theoretically block if the broker is full, but it should not happen in practice -func (s *Detector) Close() { - s.broker.ToDetector <- MsgToDetector{Quit: true} +func (s *Detector) handleMsg(msg MsgToDetector) { + if msg.Reset { + s.loudnessDetector.reset() + s.peakDetector.reset() + } + if msg.HasWeightingType { + s.loudnessDetector.weighting = weightings[WeightingType(msg.WeightingType)] + s.loudnessDetector.reset() + } + if msg.HasOversampling { + s.peakDetector.oversampling = msg.Oversampling + s.peakDetector.reset() + } + + switch data := msg.Data.(type) { + case *sointu.AudioBuffer: + buf := *data + for { + var chunk sointu.AudioBuffer + if len(s.chunkHistory) > 0 && len(s.chunkHistory) < 4410 { + l := min(len(buf), 4410-len(s.chunkHistory)) + s.chunkHistory = append(s.chunkHistory, buf[:l]...) + if len(s.chunkHistory) < 4410 { + break + } + chunk = s.chunkHistory + buf = buf[l:] + } else { + if len(buf) >= 4410 { + chunk = buf[:4410] + buf = buf[4410:] + } else { + s.chunkHistory = s.chunkHistory[:0] + s.chunkHistory = append(s.chunkHistory, buf...) + break + } + } + TrySend(s.broker.ToModel, MsgToModel{ + HasDetectorResult: true, + DetectorResult: DetectorResult{ + Loudness: s.loudnessDetector.update(chunk), + Peaks: s.peakDetector.update(chunk), + }, + }) + } + } } func makeLoudnessDetector(weighting WeightingType) loudnessDetector { diff --git a/tracker/gioui/keyevent.go b/tracker/gioui/keyevent.go index eaa990c..f7a9547 100644 --- a/tracker/gioui/keyevent.go +++ b/tracker/gioui/keyevent.go @@ -184,7 +184,7 @@ func (t *Tracker) KeyEvent(e key.Event, gtx C) { t.OpenSong().Do() case "Quit": if canQuit { - t.Quit().Do() + t.RequestQuit().Do() } case "SaveSong": t.SaveSong().Do() diff --git a/tracker/gioui/songpanel.go b/tracker/gioui/songpanel.go index 8c9e1ed..085c9d0 100644 --- a/tracker/gioui/songpanel.go +++ b/tracker/gioui/songpanel.go @@ -336,7 +336,7 @@ func NewMenuBar(model *tracker.Model) *MenuBar { {IconBytes: icons.ImageAudiotrack, Text: "Export Wav...", ShortcutText: keyActionMap["ExportWav"], Doer: model.Export()}, } if canQuit { - ret.fileMenuItems = append(ret.fileMenuItems, MenuItem{IconBytes: icons.ActionExitToApp, Text: "Quit", ShortcutText: keyActionMap["Quit"], Doer: model.Quit()}) + ret.fileMenuItems = append(ret.fileMenuItems, MenuItem{IconBytes: icons.ActionExitToApp, Text: "Quit", ShortcutText: keyActionMap["Quit"], Doer: model.RequestQuit()}) } ret.editMenuItems = []MenuItem{ {IconBytes: icons.ContentUndo, Text: "Undo", ShortcutText: keyActionMap["Undo"], Doer: model.Undo()}, diff --git a/tracker/gioui/tracker.go b/tracker/gioui/tracker.go index dcea444..030b28d 100644 --- a/tracker/gioui/tracker.go +++ b/tracker/gioui/tracker.go @@ -5,7 +5,6 @@ import ( "image" "io" "path/filepath" - "sync" "time" "gioui.org/app" @@ -50,7 +49,6 @@ type ( filePathString tracker.String - quitWG sync.WaitGroup execChan chan func() preferences Preferences @@ -102,75 +100,75 @@ func NewTracker(model *tracker.Model) *Tracker { t.Theme.Palette.Fg = primaryColor t.Theme.Palette.ContrastFg = black t.TrackEditor.scrollTable.Focus() - t.quitWG.Add(1) return t } func (t *Tracker) Main() { - titleFooter := "" - w := t.newWindow() t.InstrumentEditor.Focus() recoveryTicker := time.NewTicker(time.Second * 30) - t.Explorer = explorer.NewExplorer(w) - // Make a channel to read window events from. - events := make(chan event.Event) - // Make a channel to signal the end of processing a window event. - acks := make(chan struct{}) - go eventLoop(w, events, acks) var ops op.Ops - for { - select { - case e := <-t.Broker().ToModel: - t.ProcessMsg(e) - w.Invalidate() - case e := <-events: - switch e := e.(type) { - case app.DestroyEvent: - acks <- struct{}{} - if canQuit { - t.Quit().Do() + titlePath := "" + for !t.Quitted() { + w := t.newWindow() + w.Option(app.Title(titleFromPath(titlePath))) + t.Explorer = explorer.NewExplorer(w) + acks := make(chan struct{}) + events := make(chan event.Event) + go func() { + for { + ev := w.Event() + events <- ev + <-acks + if _, ok := ev.(app.DestroyEvent); ok { + return } - if !t.Quitted() { - // TODO: uh oh, there's no way of canceling the destroyevent in gioui? so we create a new window just to show the dialog - w = t.newWindow() - t.Explorer = explorer.NewExplorer(w) - go eventLoop(w, events, acks) - } - case app.FrameEvent: - if titleFooter != t.filePathString.Value() { - titleFooter = t.filePathString.Value() - if titleFooter != "" { - w.Option(app.Title(fmt.Sprintf("Sointu Tracker - %v", titleFooter))) - } else { - w.Option(app.Title("Sointu Tracker")) + } + }() + F: + for { + select { + case e := <-t.Broker().ToModel: + t.ProcessMsg(e) + w.Invalidate() + case <-t.Broker().CloseGUI: + t.ForceQuit().Do() + w.Perform(system.ActionClose) + case e := <-events: + switch e := e.(type) { + case app.DestroyEvent: + if canQuit { + t.RequestQuit().Do() + } + acks <- struct{}{} + break F // this window is done, we need to create a new one + case app.FrameEvent: + if titlePath != t.filePathString.Value() { + titlePath = t.filePathString.Value() + w.Option(app.Title(titleFromPath(titlePath))) + } + gtx := app.NewContext(&ops, e) + if t.Playing().Value() && t.Follow().Value() { + t.TrackEditor.scrollTable.RowTitleList.CenterOn(t.PlaySongRow()) + } + t.Layout(gtx, w) + e.Frame(gtx.Ops) + if t.Quitted() { + w.Perform(system.ActionClose) } } - gtx := app.NewContext(&ops, e) - if t.Playing().Value() && t.Follow().Value() { - t.TrackEditor.scrollTable.RowTitleList.CenterOn(t.PlaySongRow()) - } - t.Layout(gtx, w) - e.Frame(gtx.Ops) - acks <- struct{}{} - default: acks <- struct{}{} + case <-recoveryTicker.C: + t.SaveRecovery() } - case <-recoveryTicker.C: - t.SaveRecovery() - } - if t.Quitted() { - break } } recoveryTicker.Stop() - w.Perform(system.ActionClose) t.SaveRecovery() - t.quitWG.Done() + close(t.Broker().FinishedGUI) } func (t *Tracker) newWindow() *app.Window { w := new(app.Window) - w.Option(app.Title("Sointu Tracker")) w.Option(app.Size(t.preferences.WindowSize())) if t.preferences.Window.Maximized { w.Option(app.Maximized.Option()) @@ -178,21 +176,11 @@ func (t *Tracker) newWindow() *app.Window { return w } -func eventLoop(w *app.Window, events chan<- event.Event, acks <-chan struct{}) { - // Iterate window events, sending each to the old event loop and waiting for - // a signal that processing is complete before iterating again. - for { - ev := w.Event() - events <- ev - <-acks - if _, ok := ev.(app.DestroyEvent); ok { - return - } +func titleFromPath(path string) string { + if path == "" { + return "Sointu Tracker" } -} - -func (t *Tracker) WaitQuitted() { - t.quitWG.Wait() + return fmt.Sprintf("Sointu Tracker - %s", path) } func (t *Tracker) Layout(gtx layout.Context, w *app.Window) { diff --git a/tracker/model_test.go b/tracker/model_test.go index 8e41bfc..dee3f8b 100644 --- a/tracker/model_test.go +++ b/tracker/model_test.go @@ -309,6 +309,6 @@ func FuzzModel(f *testing.F) { } } closeChan <- struct{}{} - broker.ToDetector <- tracker.MsgToDetector{Quit: true} + broker.CloseDetector <- struct{}{} }) }