From 45d8d7eaf963eedc40258bb5c136d7910ee563e6 Mon Sep 17 00:00:00 2001 From: Patrick Pacher Date: Tue, 20 Oct 2020 12:40:20 +0200 Subject: [PATCH 1/6] Add StackableAnnotation and update wording --- api/config.go | 2 +- config/expertise.go | 6 +++--- config/option.go | 6 ++++++ config/release.go | 2 +- template/module.go | 2 +- 5 files changed, 12 insertions(+), 6 deletions(-) diff --git a/api/config.go b/api/config.go index 2e7c079..c373b3f 100644 --- a/api/config.go +++ b/api/config.go @@ -41,7 +41,7 @@ func registerConfig() error { err := config.Register(&config.Option{ Name: "API Address", Key: CfgDefaultListenAddressKey, - Description: "Define on which IP and port the API should listen on.", + Description: "Defines the IP address and port for the internal HTTP database API.", OptType: config.OptTypeString, ExpertiseLevel: config.ExpertiseLevelDeveloper, ReleaseLevel: config.ReleaseLevelStable, diff --git a/config/expertise.go b/config/expertise.go index bc83c32..7d7798b 100644 --- a/config/expertise.go +++ b/config/expertise.go @@ -41,7 +41,7 @@ func registerExpertiseLevelOption() { expertiseLevelOption = &Option{ Name: "Expertise Level", Key: expertiseLevelKey, - Description: "The Expertise Level controls the perceived complexity. Higher settings will show you more complex settings and information. This might also affect various other things relying on this setting. Modified settings in higher expertise levels stay in effect when switching back. (Unlike the Release Level)", + Description: "The Expertise Level controls the perceived complexity. Higher settings will enable more complex settings and information. This might also affect various other things relying on this setting. Modified settings in higher expertise levels stay in effect when switching back. (Unlike the Release Level)", OptType: OptTypeString, ExpertiseLevel: ExpertiseLevelUser, ReleaseLevel: ReleaseLevelStable, @@ -52,9 +52,9 @@ func registerExpertiseLevelOption() { }, PossibleValues: []PossibleValue{ { - Name: "Easy", + Name: "Simple", Value: ExpertiseLevelNameUser, - Description: "Easy application mode by hidding complex settings.", + Description: "Simple application mode by hidding complex settings.", }, { Name: "Expert", diff --git a/config/option.go b/config/option.go index 260d17c..5e7191b 100644 --- a/config/option.go +++ b/config/option.go @@ -84,6 +84,12 @@ const ( // SubsystemAnnotation can be used to mark an option as part // of a module subsystem. SubsystemAnnotation = "safing/portbase:module:subsystem" + // StackableAnnotation can be set on configuration options that + // stack on top of the default (or otherwise related) options. + // The value of StackableAnnotaiton is expected to be a boolean but + // may be extended to hold references to other options in the + // future. + StackableAnnotation = "safing/portbase:options:stackable" // QuickSettingAnnotation can be used to add quick settings to // a configuration option. A quick setting can support the user // by switching between pre-configured values. diff --git a/config/release.go b/config/release.go index 965ef14..ec70293 100644 --- a/config/release.go +++ b/config/release.go @@ -39,7 +39,7 @@ func registerReleaseLevelOption() { releaseLevelOption = &Option{ Name: "Release Level", Key: releaseLevelKey, - Description: "The Release Level changes which features are available to you. Some beta or experimental features are also available in the stable release channel. Unavailable settings are set to the default value.", + Description: "The Release Level changes which features are available. Some beta or experimental features are also available in the stable release channel. Unavailable settings are set to the default value.", OptType: OptTypeString, ExpertiseLevel: ExpertiseLevelExpert, ReleaseLevel: ReleaseLevelStable, diff --git a/template/module.go b/template/module.go index fa38ec0..72097ad 100644 --- a/template/module.go +++ b/template/module.go @@ -30,7 +30,7 @@ func init() { module, "config:template", // key space for configuration options registered &config.Option{ - Name: "Enable Template Subsystem", + Name: "Template Subsystem", Key: "config:subsystems/template", Description: "This option enables the Template Subsystem [TEMPLATE]", OptType: config.OptTypeBool, From 3f3305d8c2ea244fa106c490c884bab48f451d47 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 29 Oct 2020 12:30:15 +0100 Subject: [PATCH 2/6] Improve config naming --- api/config.go | 2 +- config/expertise.go | 10 +++++----- config/release.go | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api/config.go b/api/config.go index c373b3f..7d8c7bf 100644 --- a/api/config.go +++ b/api/config.go @@ -41,7 +41,7 @@ func registerConfig() error { err := config.Register(&config.Option{ Name: "API Address", Key: CfgDefaultListenAddressKey, - Description: "Defines the IP address and port for the internal HTTP database API.", + Description: "Defines the IP address and port for the internal API.", OptType: config.OptTypeString, ExpertiseLevel: config.ExpertiseLevelDeveloper, ReleaseLevel: config.ReleaseLevelStable, diff --git a/config/expertise.go b/config/expertise.go index 7d7798b..78acd29 100644 --- a/config/expertise.go +++ b/config/expertise.go @@ -39,9 +39,9 @@ func init() { func registerExpertiseLevelOption() { expertiseLevelOption = &Option{ - Name: "Expertise Level", + Name: "UI Mode", Key: expertiseLevelKey, - Description: "The Expertise Level controls the perceived complexity. Higher settings will enable more complex settings and information. This might also affect various other things relying on this setting. Modified settings in higher expertise levels stay in effect when switching back. (Unlike the Release Level)", + Description: "Controls the amount of settings and information shown. Hidden settings are still in effect - unlike with the Release Level.", OptType: OptTypeString, ExpertiseLevel: ExpertiseLevelUser, ReleaseLevel: ReleaseLevelStable, @@ -54,12 +54,12 @@ func registerExpertiseLevelOption() { { Name: "Simple", Value: ExpertiseLevelNameUser, - Description: "Simple application mode by hidding complex settings.", + Description: "Hide complex settings and information.", }, { - Name: "Expert", + Name: "Advanced", Value: ExpertiseLevelNameExpert, - Description: "Expert application mode. Allows access to almost all configuration options.", + Description: "Show technical details.", }, { Name: "Developer", diff --git a/config/release.go b/config/release.go index ec70293..09401b4 100644 --- a/config/release.go +++ b/config/release.go @@ -39,7 +39,7 @@ func registerReleaseLevelOption() { releaseLevelOption = &Option{ Name: "Release Level", Key: releaseLevelKey, - Description: "The Release Level changes which features are available. Some beta or experimental features are also available in the stable release channel. Unavailable settings are set to the default value.", + Description: "Controls the amount of available settings. Hidden settings revert to default - unlike with the UI Mode.", OptType: OptTypeString, ExpertiseLevel: ExpertiseLevelExpert, ReleaseLevel: ReleaseLevelStable, @@ -62,7 +62,7 @@ func registerReleaseLevelOption() { { Name: "Experimental", Value: ReleaseLevelNameExperimental, - Description: "Show experimental features", + Description: "Show all features", }, }, } From 8a0c3a077cb389ff8cd18fb14d43476c21acb5ce Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 29 Oct 2020 13:58:33 +0100 Subject: [PATCH 3/6] Fix linter errors and update deps --- config/option.go | 2 +- config/validate.go | 2 +- database/hook.go | 2 +- modules/subsystems/module.go | 16 ++++++++-------- modules/subsystems/registry.go | 6 +++--- modules/subsystems/subsystem.go | 2 +- runtime/module_api.go | 4 +--- runtime/registry.go | 2 +- runtime/registry_test.go | 6 +++--- 9 files changed, 20 insertions(+), 22 deletions(-) diff --git a/config/option.go b/config/option.go index 5e7191b..0a5ac5c 100644 --- a/config/option.go +++ b/config/option.go @@ -177,7 +177,7 @@ type Option struct { // been created. Description string // Help may hold a long version of the description providing - // assistence with the configuration option. + // assistance with the configuration option. // Help is considered immutable after the option has // been created. Help string diff --git a/config/validate.go b/config/validate.go index 0138379..58fd319 100644 --- a/config/validate.go +++ b/config/validate.go @@ -45,7 +45,7 @@ func isAllowedPossibleValue(opt *Option, value interface{}) error { compareAgainst := val.Value valueType := reflect.TypeOf(value) - // loading int's from the configuration JSON does not perserve the correct type + // loading int's from the configuration JSON does not preserve the correct type // as we get float64 instead. Make sure to convert them before. if reflect.TypeOf(val.Value).ConvertibleTo(valueType) { compareAgainst = reflect.ValueOf(val.Value).Convert(valueType).Interface() diff --git a/database/hook.go b/database/hook.go index f395ef0..e4a1d13 100644 --- a/database/hook.go +++ b/database/hook.go @@ -17,7 +17,7 @@ type Hook interface { // the underlying storage. A PreGet hookd may be used to // implement more advanced access control on database keys. PreGet(dbKey string) error - // UsesPostGet should returnd true if the hook's PostGet + // UsesPostGet should return true if the hook's PostGet // should be called after loading a database record from // the underlying storage. UsesPostGet() bool diff --git a/modules/subsystems/module.go b/modules/subsystems/module.go index 406ed53..539fe77 100644 --- a/modules/subsystems/module.go +++ b/modules/subsystems/module.go @@ -21,7 +21,7 @@ var ( printGraphFlag bool ) -// Register registeres a new subsystem. It's like Manager.Register +// Register registers a new subsystem. It's like Manager.Register // but uses DefaultManager and panics on error. func Register(id, name, description string, module *modules.Module, configKeySpace string, option *config.Option) { err := DefaultManager.Register(id, name, description, module, configKeySpace, option) @@ -92,7 +92,7 @@ func prep() error { func start() error { // Registration of subsystems is only allowed during - // preperation. Make sure any further call to Register() + // preparation. Make sure any further call to Register() // panics. if err := DefaultManager.Start(); err != nil { return err @@ -104,9 +104,9 @@ func start() error { } // PrintGraph prints the subsystem and module graph. -func (reg *Manager) PrintGraph() { - reg.l.RLock() - defer reg.l.RUnlock() +func (mng *Manager) PrintGraph() { + mng.l.RLock() + defer mng.l.RUnlock() fmt.Println("subsystems dependency graph:") @@ -114,17 +114,17 @@ func (reg *Manager) PrintGraph() { module.Disable() // mark roots - for _, sub := range reg.subsys { + for _, sub := range mng.subsys { sub.module.Enable() // mark as tree root } - for _, sub := range reg.subsys { + for _, sub := range mng.subsys { printModuleGraph("", sub.module, true) } fmt.Println("\nsubsystem module groups:") _ = start() // no errors for what we need here - for _, sub := range reg.subsys { + for _, sub := range mng.subsys { fmt.Printf("├── %s\n", sub.Name) for _, mod := range sub.Modules[1:] { fmt.Printf("│ ├── %s\n", mod.Name) diff --git a/modules/subsystems/registry.go b/modules/subsystems/registry.go index 06aa040..1da1587 100644 --- a/modules/subsystems/registry.go +++ b/modules/subsystems/registry.go @@ -36,7 +36,7 @@ type Manager struct { runtime *runtime.Registry } -// NewManager returns a new subsystem manager that registeres +// NewManager returns a new subsystem manager that registers // itself at rtReg. func NewManager(rtReg *runtime.Registry) (*Manager, error) { mng := &Manager{ @@ -71,7 +71,7 @@ func (mng *Manager) Start() error { } // aggregate all modules dependencies (and the subsystem module itself) - // into the Modules slice. Configuration options form dependened modules + // into the Modules slice. Configuration options form dependent modules // will be marked using config.SubsystemAnnotation if not already set. for _, sub := range mng.subsys { sub.Modules = append(sub.Modules, statusFromModule(sub.module)) @@ -118,7 +118,7 @@ func (mng *Manager) Get(keyOrPrefix string) ([]record.Record, error) { return records, nil } -// Register registeres a new subsystem. The given option must be a bool option. +// Register registers a new subsystem. The given option must be a bool option. // Should be called in init() directly after the modules.Register() function. // The config option must not yet be registered and will be registered for // you. Pass a nil option to force enable. diff --git a/modules/subsystems/subsystem.go b/modules/subsystems/subsystem.go index f317c6b..ccfb320 100644 --- a/modules/subsystems/subsystem.go +++ b/modules/subsystems/subsystem.go @@ -28,7 +28,7 @@ type Subsystem struct { //nolint:maligned // not worth the effort // FailureStatus is the worst failure status that is currently // set in one of the subsystem's dependencies. FailureStatus uint8 - // ToggleOptionKey holds the key of the configuraiton option + // ToggleOptionKey holds the key of the configuration option // that is used to completely enable or disable this subsystem. ToggleOptionKey string // ExpertiseLevel defines the complexity of the subsystem and is diff --git a/runtime/module_api.go b/runtime/module_api.go index 58516a6..2316c91 100644 --- a/runtime/module_api.go +++ b/runtime/module_api.go @@ -9,12 +9,10 @@ var ( // DefaultRegistry is the default registry // that is used by the module-level API. DefaultRegistry = NewRegistry() - - module *modules.Module ) func init() { - module = modules.Register("runtime", nil, startModule, nil, "database") + modules.Register("runtime", nil, startModule, nil, "database") } func startModule() error { diff --git a/runtime/registry.go b/runtime/registry.go index fc683ca..7c3404b 100644 --- a/runtime/registry.go +++ b/runtime/registry.go @@ -36,7 +36,7 @@ var ( // package but may consider creating a dedicated // runtime registry on their own. Registry uses // a radix tree for value providers and their -// choosen database key/prefix. +// chosen database key/prefix. type Registry struct { l sync.RWMutex providers *radix.Tree diff --git a/runtime/registry_test.go b/runtime/registry_test.go index 458a7fb..61343da 100644 --- a/runtime/registry_test.go +++ b/runtime/registry_test.go @@ -43,7 +43,7 @@ func getTestRegistry(t *testing.T) *Registry { r := NewRegistry() providers := []testProvider{ - testProvider{ + { k: "p1/", r: []record.Record{ makeTestRecord("p1/f1/v1", "p1.1"), @@ -51,7 +51,7 @@ func getTestRegistry(t *testing.T) *Registry { makeTestRecord("p1/v3", "p1.3"), }, }, - testProvider{ + { k: "p2/f1", r: []record.Record{ makeTestRecord("p2/f1/v1", "p2.1"), @@ -104,7 +104,7 @@ func TestRegistryQuery(t *testing.T) { iter, err := reg.Query(q, true, true) require.NoError(t, err) require.NotNil(t, iter) - var records []record.Record + var records []record.Record //nolint:prealloc for r := range iter.Next { records = append(records, r) } From dbbe556f3c97415d25bea9151b4061c260169a00 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 29 Oct 2020 13:58:52 +0100 Subject: [PATCH 4/6] Add Has fn to config perspective --- config/option.go | 3 +++ config/perspective.go | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/config/option.go b/config/option.go index 0a5ac5c..e391768 100644 --- a/config/option.go +++ b/config/option.go @@ -16,6 +16,7 @@ type OptionType uint8 // Various attribute options. Use ExternalOptType for extended types in the frontend. const ( + optTypeAny OptionType = 0 OptTypeString OptionType = 1 OptTypeStringArray OptionType = 2 OptTypeInt OptionType = 3 @@ -24,6 +25,8 @@ const ( func getTypeName(t OptionType) string { switch t { + case optTypeAny: + return "any" case OptTypeString: return "string" case OptTypeStringArray: diff --git a/config/perspective.go b/config/perspective.go index ea2b25d..ba4b428 100644 --- a/config/perspective.go +++ b/config/perspective.go @@ -75,7 +75,7 @@ func (p *Perspective) getPerspectiveValueCache(name string, requestedType Option } // check type - if requestedType != pOption.option.OptType { + if requestedType != pOption.option.OptType && requestedType != optTypeAny { log.Errorf("config: bad type: requested %s as %s, but is %s", name, getTypeName(requestedType), getTypeName(pOption.option.OptType)) return nil } @@ -88,6 +88,12 @@ func (p *Perspective) getPerspectiveValueCache(name string, requestedType Option return pOption.valueCache } +// Has returns whether the given option is set in the perspective. +func (p *Perspective) Has(name string) bool { + valueCache := p.getPerspectiveValueCache(name, optTypeAny) + return valueCache != nil +} + // GetAsString returns a function that returns the wanted string with high performance. func (p *Perspective) GetAsString(name string) (value string, ok bool) { valueCache := p.getPerspectiveValueCache(name, OptTypeString) From 1038e875dadd36623d9d8ca13539551da2045b38 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 29 Oct 2020 13:59:12 +0100 Subject: [PATCH 5/6] Clean up notification storage --- notifications/cleaner.go | 55 +++++++------- notifications/database.go | 81 ++++++++++++--------- notifications/notification.go | 130 +++++++++++++++++++++------------- 3 files changed, 154 insertions(+), 112 deletions(-) diff --git a/notifications/cleaner.go b/notifications/cleaner.go index 436a9c9..f9e2ffd 100644 --- a/notifications/cleaner.go +++ b/notifications/cleaner.go @@ -3,48 +3,49 @@ package notifications import ( "context" "time" - - "github.com/safing/portbase/log" ) -func cleaner(ctx context.Context) error { - ticker := time.NewTicker(5 * time.Second) +func cleaner(ctx context.Context) error { //nolint:unparam // Conforms to worker interface + ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() -L: for { select { case <-ctx.Done(): - break L + return nil case <-ticker.C: deleteExpiredNotifs() } } - return nil } func deleteExpiredNotifs() { - now := time.Now().Unix() + // Get a copy of the notification map. + notsCopy := getNotsCopy() - notsLock.Lock() - defer notsLock.Unlock() - - toDelete := make([]*Notification, 0, len(nots)) - for _, n := range nots { - n.Lock() - if now > n.Expires { - toDelete = append(toDelete, n) - } - n.Unlock() - } - - for _, n := range toDelete { - n.Lock() - err := n.delete(true) - n.Unlock() - - if err != nil { - log.Debugf("notifications: failed to delete %s: %s", n.EventID, err) + // Delete all expired notifications. + for _, n := range notsCopy { + if n.isExpired() { + n.delete(true) } } } + +func (n *Notification) isExpired() bool { + n.Lock() + defer n.Unlock() + + return n.Expires > 0 && n.Expires < time.Now().Unix() +} + +func getNotsCopy() []*Notification { + notsLock.RLock() + defer notsLock.RUnlock() + + notsCopy := make([]*Notification, 0, len(nots)) + for _, n := range nots { + notsCopy = append(notsCopy, n) + } + + return notsCopy +} diff --git a/notifications/database.go b/notifications/database.go index a06c2f3..f9d8be7 100644 --- a/notifications/database.go +++ b/notifications/database.go @@ -54,21 +54,27 @@ func registerAsDatabase() error { // Get returns a database record. func (s *StorageInterface) Get(key string) (record.Record, error) { - notsLock.RLock() - defer notsLock.RUnlock() - - // transform key + // Get EventID from key. if !strings.HasPrefix(key, "all/") { return nil, storage.ErrNotFound } key = strings.TrimPrefix(key, "all/") - // get notification - not, ok := nots[key] + // Get notification from storage. + n, ok := getNotification(key) if !ok { return nil, storage.ErrNotFound } - return not, nil + + return n, nil +} + +func getNotification(eventID string) (n *Notification, ok bool) { + notsLock.RLock() + defer notsLock.RUnlock() + + n, ok = nots[eventID] + return } // Query returns a an iterator for the supplied query. @@ -81,16 +87,12 @@ func (s *StorageInterface) Query(q *query.Query, local, internal bool) (*iterato } func (s *StorageInterface) processQuery(q *query.Query, it *iterator.Iterator) { - notsLock.RLock() - defer notsLock.RUnlock() + // Get a copy of the notification map. + notsCopy := getNotsCopy() // send all notifications - for _, n := range nots { - if n.Meta().IsDeleted() { - continue - } - - if q.MatchesKey(n.DatabaseKey()) && q.MatchesRecord(n) { + for _, n := range notsCopy { + if inQuery(n, q) { select { case it.Next <- n: case <-it.Done: @@ -103,6 +105,22 @@ func (s *StorageInterface) processQuery(q *query.Query, it *iterator.Iterator) { it.Finish(nil) } +func inQuery(n *Notification, q *query.Query) bool { + n.lock.Lock() + defer n.lock.Unlock() + + switch { + case n.Meta().IsDeleted(): + return false + case !q.MatchesKey(n.DatabaseKey()): + return false + case !q.MatchesRecord(n): + return false + } + + return true +} + // Put stores a record in the database. func (s *StorageInterface) Put(r record.Record) (record.Record, error) { // record is already locked! @@ -125,12 +143,9 @@ func (s *StorageInterface) Put(r record.Record) (record.Record, error) { func applyUpdate(n *Notification, key string) (*Notification, error) { // separate goroutine in order to correctly lock notsLock - notsLock.RLock() - existing, ok := nots[key] - notsLock.RUnlock() + existing, ok := getNotification(key) // ignore if already deleted - if !ok || existing.Meta().IsDeleted() { // this is a completely new notification // we pass pushUpdate==false because the storage @@ -139,6 +154,14 @@ func applyUpdate(n *Notification, key string) (*Notification, error) { return n, nil } + // Save when we're finished, if needed. + save := false + defer func() { + if save { + existing.save(false) + } + }() + existing.Lock() defer existing.Unlock() @@ -146,8 +169,6 @@ func applyUpdate(n *Notification, key string) (*Notification, error) { return existing, fmt.Errorf("action already executed") } - save := false - // check if the notification has been marked as // "executed externally". if n.State == Executed { @@ -171,33 +192,25 @@ func applyUpdate(n *Notification, key string) (*Notification, error) { save = true } - if save { - existing.save(false) - } - return existing, nil } // Delete deletes a record from the database. func (s *StorageInterface) Delete(key string) error { - // transform key + // Get EventID from key. if !strings.HasPrefix(key, "all/") { return storage.ErrNotFound } key = strings.TrimPrefix(key, "all/") - notsLock.Lock() - defer notsLock.Unlock() - - n, ok := nots[key] + // Get notification from storage. + n, ok := getNotification(key) if !ok { return storage.ErrNotFound } - n.Lock() - defer n.Unlock() - - return n.delete(true) + n.delete(true) + return nil } // ReadOnly returns whether the database is read only. diff --git a/notifications/notification.go b/notifications/notification.go index 1b3cf52..aa6484c 100644 --- a/notifications/notification.go +++ b/notifications/notification.go @@ -1,6 +1,7 @@ package notifications import ( + "context" "fmt" "sync" "time" @@ -23,6 +24,10 @@ const ( // State describes the state of a notification. type State string +// NotificationActionFn defines the function signature for notification action +// functions. +type NotificationActionFn func(context.Context, *Notification) error + // Possible notification states. // State transitions can only happen from top to bottom. const ( @@ -81,9 +86,9 @@ type Notification struct { SelectedActionID string lock sync.Mutex - actionFunction func(*Notification) // call function to process action - actionTrigger chan string // and/or send to a channel - expiredTrigger chan struct{} // closed on expire + actionFunction NotificationActionFn // call function to process action + actionTrigger chan string // and/or send to a channel + expiredTrigger chan struct{} // closed on expire } // Action describes an action that can be taken for a notification. @@ -92,8 +97,6 @@ type Action struct { Text string } -func noOpAction(n *Notification) {} - // Get returns the notification identifed by the given id or nil if it doesn't exist. func Get(id string) *Notification { notsLock.RLock() @@ -149,18 +152,34 @@ func notify(nType Type, id string, msg string, actions ...Action) *Notification // Save saves the notification and returns it. func (n *Notification) Save() *Notification { - n.Lock() - defer n.Unlock() - return n.save(true) } func (n *Notification) save(pushUpdate bool) *Notification { + var id string + + // Delete notification after processing deletion. + defer func() { + // Lock and delete from notification storage. + notsLock.Lock() + defer notsLock.Unlock() + nots[id] = n + }() + + // We do not access EventData here, so it is enough to just lock the + // notification itself. + n.lock.Lock() + defer n.lock.Unlock() + + // Save ID for deletion + id = n.EventID + + // Generate random GUID if not set. if n.GUID == "" { n.GUID = utils.RandomUUID(n.EventID).String() } - // make ack notification if there are no defined actions + // Make ack notification if there are no defined actions. if len(n.AvailableActions) == 0 { n.AvailableActions = []*Action{ { @@ -168,12 +187,6 @@ func (n *Notification) save(pushUpdate bool) *Notification { Text: "OK", }, } - n.actionFunction = noOpAction - } - - // Make sure we always have a reasonable expiration set. - if n.Expires == 0 { - n.Expires = time.Now().Add(72 * time.Hour).Unix() } // Make sure we always have a notification state assigned. @@ -182,17 +195,14 @@ func (n *Notification) save(pushUpdate bool) *Notification { } // check key - if n.DatabaseKey() == "" { + if !n.KeyIsSet() { n.SetKey(fmt.Sprintf("notifications:all/%s", n.EventID)) } + // Update meta data. n.UpdateMeta() - // store the notification inside or map - notsLock.Lock() - nots[n.EventID] = n - notsLock.Unlock() - + // Push update via the database system if needed. if pushUpdate { log.Tracef("notifications: pushing update for %s to subscribers", n.Key()) dbController.PushUpdate(n) @@ -203,7 +213,7 @@ func (n *Notification) save(pushUpdate bool) *Notification { // SetActionFunction sets a trigger function to be executed when the user reacted on the notification. // The provided function will be started as its own goroutine and will have to lock everything it accesses, even the provided notification. -func (n *Notification) SetActionFunction(fn func(*Notification)) *Notification { +func (n *Notification) SetActionFunction(fn NotificationActionFn) *Notification { n.lock.Lock() defer n.lock.Unlock() n.actionFunction = fn @@ -224,43 +234,70 @@ func (n *Notification) Response() <-chan string { // Update updates/resends a notification if it was not already responded to. func (n *Notification) Update(expires int64) { + // Save when we're finished, if needed. + save := false + defer func() { + if save { + n.save(true) + } + }() + n.lock.Lock() defer n.lock.Unlock() - if n.State == Active { - n.Expires = expires - n.save(true) + // Don't update if notification isn't active. + if n.State != Active { + return } + + // Don't update too quickly. + if n.Meta().Modified > time.Now().Add(-10*time.Second).Unix() { + return + } + + // Update expiry and save. + n.Expires = expires + save = true } // Delete (prematurely) cancels and deletes a notification. func (n *Notification) Delete() error { - notsLock.Lock() - defer notsLock.Unlock() - n.Lock() - defer n.Unlock() - - return n.delete(true) + n.delete(true) + return nil } -func (n *Notification) delete(pushUpdate bool) error { - // mark as deleted +func (n *Notification) delete(pushUpdate bool) { + var id string + + // Delete notification after processing deletion. + defer func() { + // Lock and delete from notification storage. + notsLock.Lock() + defer notsLock.Unlock() + delete(nots, id) + }() + + // We do not access EventData here, so it is enough to just lock the + // notification itself. + n.lock.Lock() + defer n.lock.Unlock() + + // Save ID for deletion + id = n.EventID + + // Mark notification as deleted. n.Meta().Delete() - // delete from internal storage - delete(nots, n.EventID) - - // close expired + // Close expiry channel if available. if n.expiredTrigger != nil { close(n.expiredTrigger) n.expiredTrigger = nil } + // Push update via the database system if needed. if pushUpdate { dbController.PushUpdate(n) } - - return nil } // Expired notifies the caller when the notification has expired. @@ -286,7 +323,9 @@ func (n *Notification) selectAndExecuteAction(id string) { executed := false if n.actionFunction != nil { - go n.actionFunction(n) + module.StartWorker("notification action execution", func(ctx context.Context) error { + return n.actionFunction(ctx, n) + }) executed = true } @@ -336,14 +375,3 @@ func (n *Notification) Unlock() { locker.Unlock() } } - -func duplicateActions(original []*Action) (duplicate []*Action) { - duplicate = make([]*Action, len(original)) - for _, action := range original { - duplicate = append(duplicate, &Action{ - ID: action.ID, - Text: action.Text, - }) - } - return -} From a1e5046996e532770f20b70b95c29c9410f63518 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 29 Oct 2020 16:10:52 +0100 Subject: [PATCH 6/6] Improve documentation --- notifications/notification.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/notifications/notification.go b/notifications/notification.go index aa6484c..b649cc9 100644 --- a/notifications/notification.go +++ b/notifications/notification.go @@ -14,7 +14,7 @@ import ( // Type describes the type of a notification. type Type uint8 -// Notification types +// Notification types. const ( Info Type = 0 Warning Type = 1 @@ -155,12 +155,14 @@ func (n *Notification) Save() *Notification { return n.save(true) } +// save saves the notification to the internal storage. It locks the +// notification, so it must not be locked when save is called. func (n *Notification) save(pushUpdate bool) *Notification { var id string // Delete notification after processing deletion. defer func() { - // Lock and delete from notification storage. + // Lock and save to notification storage. notsLock.Lock() defer notsLock.Unlock() nots[id] = n @@ -266,6 +268,8 @@ func (n *Notification) Delete() error { return nil } +// delete deletes the notification from the internal storage. It locks the +// notification, so it must not be locked when delete is called. func (n *Notification) delete(pushUpdate bool) { var id string