From 656be379e46e14dd5fa4154210562037747b677d Mon Sep 17 00:00:00 2001 From: Sasha Koshka Date: Thu, 22 Aug 2024 16:03:05 -0400 Subject: [PATCH] Fix goroutine issues with config --- config/impl.go | 102 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 70 insertions(+), 32 deletions(-) diff --git a/config/impl.go b/config/impl.go index 21529f7..c7d0516 100644 --- a/config/impl.go +++ b/config/impl.go @@ -8,9 +8,16 @@ import "path/filepath" import "github.com/fsnotify/fsnotify" import "git.tebibyte.media/tomo/tomo/event" +// Goroutine model: +// All private methods (except for lockAndProcessEvent) do not lock the config, +// but all public methods do. Private methods may not call public methods. +// Locking must always be method-level, with a call to Lock at the start, +// directly followed by a deferred call to Unlock. + type config struct { open bool watcher *fsnotify.Watcher + lock sync.RWMutex paths struct { user string @@ -19,12 +26,11 @@ type config struct { } data struct { - user *File + user *File system []map[string] Value } on struct { - lock sync.RWMutex change event.Broadcaster[func (string)] } } @@ -45,10 +51,33 @@ func NewConfig (user string, system ...string) (ConfigCloser, error) { conf.paths.system = system err := conf.init() if err != nil { return nil, err } - go conf.run() + go func () { + for event := range conf.watcher.Events { + conf.lockAndProcessEvent(event) + } + } () return conf, nil } +// this method may only be run in the goroutine spawned by NewConfig. +func (this *config) lockAndProcessEvent (event fsnotify.Event) { + this.lock.Lock() + defer this.lock.Unlock() + if !(event.Has(fsnotify.Write)) { return } + if _, ok := this.paths.watching[event.Name]; !ok { return } + + if event.Name == this.paths.user { + this.reloadUser() + } else { + index := slices.Index(this.paths.system, event.Name) + if index > 0 { + this.reloadSystem(index) + } + } + + // TODO diff and call event handler if changed +} + func (this *config) init () error { watcher, err := fsnotify.NewWatcher() if err != nil { return err } @@ -67,29 +96,6 @@ func (this *config) init () error { return nil } -func (this *config) run () { - for event := range this.watcher.Events { - if !(event.Has(fsnotify.Write)) { continue } - if _, ok := this.paths.watching[event.Name]; !ok { continue } - - if event.Name == this.paths.user { - this.reloadUser() - } else { - index := slices.Index(this.paths.system, event.Name) - if index > 0 { - this.reloadSystem(index) - } - } - - // TODO diff and call event handler if changed - } -} - -func (this *config) Close () error { - this.open = false - return this.watcher.Close() -} - func (this *config) errIfClosed () error { if this.open { return nil @@ -140,7 +146,15 @@ func (this *config) saveUser () error { return nil } -func (this *config) Get (key string, fallback Value) (Value, error) { +func (this *config) processUserDiff (changed []string) { + // TODO +} + +func (this *config) processSystemDiff (index int, changed []string) { + // TODO +} + +func (this *config) get (key string, fallback Value) (Value, error) { // try user config if !KeyValid(key) { return nil, ErrMalformedKey } @@ -160,10 +174,26 @@ func (this *config) Get (key string, fallback Value) (Value, error) { // use fallback return fallback, nil + +} + +func (this *config) Get (key string, fallback Value) (Value, error) { + this.lock.Lock() + defer this.lock.Unlock() + return this.get(key, fallback) +} + +func (this *config) Close () error { + this.lock.Lock() + defer this.lock.Unlock() + this.open = false + return this.watcher.Close() } func (this *config) GetString (key string, fallback string) (string, error) { - value, err := this.Get(key, ValueString(fallback)) + this.lock.Lock() + defer this.lock.Unlock() + value, err := this.get(key, ValueString(fallback)) if err != nil { return "", err } if value, ok := value.(ValueString); ok { return string(value), nil @@ -172,7 +202,9 @@ func (this *config) GetString (key string, fallback string) (string, error) { } func (this *config) GetNumber (key string, fallback float64) (float64, error) { - value, err := this.Get(key, ValueNumber(fallback)) + this.lock.Lock() + defer this.lock.Unlock() + value, err := this.get(key, ValueNumber(fallback)) if err != nil { return 0, err } if value, ok := value.(ValueNumber); ok { return float64(value), nil @@ -181,7 +213,9 @@ func (this *config) GetNumber (key string, fallback float64) (float64, error) { } func (this *config) GetBool (key string, fallback bool) (bool, error) { - value, err := this.Get(key, ValueBool(fallback)) + this.lock.Lock() + defer this.lock.Unlock() + value, err := this.get(key, ValueBool(fallback)) if err != nil { return false, err } if value, ok := value.(ValueBool); ok { return bool(value), nil @@ -190,6 +224,8 @@ func (this *config) GetBool (key string, fallback bool) (bool, error) { } func (this *config) Set (key string, value Value) error { + this.lock.Lock() + defer this.lock.Unlock() if this.data.user == nil { this.data.user = NewFile() } err := this.data.user.Set(key, value) if err != nil { return err } @@ -197,6 +233,8 @@ func (this *config) Set (key string, value Value) error { } func (this *config) Reset (key string) error { + this.lock.Lock() + defer this.lock.Unlock() if this.data.user == nil { this.data.user = NewFile() } err := this.data.user.Reset(key) if err != nil { return err } @@ -204,7 +242,7 @@ func (this *config) Reset (key string) error { } func (this *config) OnChange (callback func (string)) event.Cookie { - this.on.lock.Lock() - defer this.on.lock.Unlock() + this.lock.Lock() + defer this.lock.Unlock() return this.on.change.Connect(callback) }