Merge pull request #74 from safing/fix/notifications

Drop support for peristent notifications. Fixes #71

notifications/database.go

@@ -19,9 +19,6 @@ var (
 	notsLock sync.RWMutex
 
 	dbController *database.Controller
-	dbInterface  *database.Interface
-
-	persistentBasePath string
 )
 
 // Storage interface errors
@@ -31,13 +28,6 @@ var (
 	ErrNoDelete    = errors.New("notifications may not be deleted, they must be handled")
 )
 
-// SetPersistenceBasePath sets the base path for persisting persistent notifications.
-func SetPersistenceBasePath(dbBasePath string) {
-	if persistentBasePath == "" {
-		persistentBasePath = dbBasePath
-	}
-}
-
 // StorageInterface provices a storage.Interface to the configuration manager.
 type StorageInterface struct {
 	storage.InjectBase
@@ -69,18 +59,17 @@ func (s *StorageInterface) Get(key string) (record.Record, error) {
 	defer notsLock.RUnlock()
 
 	// transform key
-	if strings.HasPrefix(key, "all/") {
+	if !strings.HasPrefix(key, "all/") {
-		key = strings.TrimPrefix(key, "all/")
-	} else {
 		return nil, storage.ErrNotFound
 	}
+	key = strings.TrimPrefix(key, "all/")
 
 	// get notification
 	not, ok := nots[key]
-	if ok {
+	if !ok {
-		return not, nil
+		return nil, storage.ErrNotFound
 	}
-	return nil, storage.ErrNotFound
+	return not, nil
 }
 
 // Query returns a an iterator for the supplied query.
@@ -103,7 +92,12 @@ func (s *StorageInterface) processQuery(q *query.Query, it *iterator.Iterator) {
 		}
 
 		if q.MatchesKey(n.DatabaseKey()) && q.MatchesRecord(n) {
-			it.Next <- n
+			select {
+			case it.Next <- n:
+			case <-it.Done:
+				// make sure we don't leak this goroutine if the iterator get's cancelled
+				return
+			}
 		}
 	}
 
@@ -127,56 +121,68 @@ func (s *StorageInterface) Put(r record.Record) (record.Record, error) {
 		return nil, ErrInvalidPath
 	}
 
-	// continue in goroutine
+	return applyUpdate(n, key)
-	go UpdateNotification(n, key)
-
-	return n, nil
 }
 
-// UpdateNotification updates a notification with input from a database action. Notification will not be saved/propagated if there is no valid change.
+func applyUpdate(n *Notification, key string) (*Notification, error) {
-func UpdateNotification(n *Notification, key string) {
-	n.Lock()
-	defer n.Unlock()
-
 	// separate goroutine in order to correctly lock notsLock
 	notsLock.RLock()
-	origN, ok := nots[key]
+	existing, ok := nots[key]
 	notsLock.RUnlock()
 
+	// ignore if already deleted
+
+	if !ok || existing.Meta().IsDeleted() {
+		// this is a completely new notification
+		// we pass pushUpdate==false because the storage
+		// controller will push an update on put anyway.
+		n.save(false)
+		return n, nil
+	}
+
+	existing.Lock()
+	defer existing.Unlock()
+
+	// A notification can only be updated to select and execute the
+	// notification action. If the existing one already has an
+	// Executed timestamp this update request is invalid
+	if existing.Executed > 0 {
+		return existing, fmt.Errorf("action already executed at %d", existing.Executed)
+	}
+
 	save := false
 
-	// ignore if already deleted
+	// check if the notification has been marked as
-	if ok && origN.Meta().IsDeleted() {
+	// "executed externally".
-		ok = false
+	if n.Executed > 0 {
-	}
-
-	if ok {
-		// existing notification
-		// only update select attributes
-		origN.Lock()
-		defer origN.Unlock()
-	} else {
-		// new notification (from external source): old == new
-		origN = n
-		save = true
-	}
-
-	switch {
-	case n.SelectedActionID != "" && n.Responded == 0:
-		// select action, if not yet already handled
-		log.Tracef("notifications: selected action for %s: %s", n.ID, n.SelectedActionID)
-		origN.selectAndExecuteAction(n.SelectedActionID)
-		save = true
-	case origN.Executed == 0 && n.Executed != 0:
 		log.Tracef("notifications: action for %s executed externally", n.ID)
-		origN.Executed = n.Executed
+		existing.Executed = n.Executed
+		save = true
+
+		// in case the action has been executed immediately by the
+		// sender we may need to update the SelectedActionID and the
+		// Responded timestamp as well. Though, we guard the assignments
+		// with value checks so partial updates that only change the
+		// Executed property do not overwrite existing values.
+		if n.SelectedActionID != "" {
+			existing.SelectedActionID = n.SelectedActionID
+		}
+		if n.Responded != 0 {
+			existing.Responded = n.Responded
+		}
+	}
+
+	if n.SelectedActionID != "" && existing.Responded == 0 {
+		log.Tracef("notifications: selected action for %s: %s", n.ID, n.SelectedActionID)
+		existing.selectAndExecuteAction(n.SelectedActionID)
 		save = true
 	}
 
 	if save {
-		// we may be locking
+		existing.save(false)
-		go origN.Save()
 	}
+
+	return existing, nil
 }
 
 // Delete deletes a record from the database.

notifications/notification.go

@@ -5,17 +5,19 @@ import (
 	"sync"
 	"time"
 
-	"github.com/safing/portbase/database"
 	"github.com/safing/portbase/database/record"
 	"github.com/safing/portbase/log"
 	"github.com/safing/portbase/utils"
 )
 
+// Type describes the type of a notification.
+type Type uint8
+
 // Notification types
 const (
-	Info    uint8 = 0
+	Info    Type = 0
-	Warning uint8 = 1
+	Warning Type = 1
-	Prompt  uint8 = 2
+	Prompt  Type = 2
 )
 
 // Notification represents a notification that is to be delivered to the user.
@@ -29,13 +31,12 @@ type Notification struct {
 	// MessageTemplate string
 	// MessageData []string
 	DataSubject sync.Locker
-	Type        uint8
+	Type        Type
 
-	Persistent bool  // this notification persists until it is handled and survives restarts
+	Created   int64 // creation timestamp, notification "starts"
-	Created    int64 // creation timestamp, notification "starts"
+	Expires   int64 // expiry timestamp, notification is expected to be canceled at this time and may be cleaned up afterwards
-	Expires    int64 // expiry timestamp, notification is expected to be canceled at this time and may be cleaned up afterwards
+	Responded int64 // response timestamp, notification "ends"
-	Responded  int64 // response timestamp, notification "ends"
+	Executed  int64 // execution timestamp, notification will be deleted soon
-	Executed   int64 // execution timestamp, notification will be deleted soon
 
 	AvailableActions []*Action
 	SelectedActionID string
@@ -52,8 +53,7 @@ type Action struct {
 	Text string
 }
 
-func noOpAction(n *Notification) {
+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 {
@@ -87,7 +87,7 @@ func NotifyPrompt(id, msg string, actions ...Action) *Notification {
 	return notify(Prompt, id, msg, actions...)
 }
 
-func notify(nType uint8, id string, msg string, actions ...Action) *Notification {
+func notify(nType Type, id string, msg string, actions ...Action) *Notification {
 	acts := make([]*Action, len(actions))
 	for idx := range actions {
 		a := actions[idx]
@@ -110,11 +110,13 @@ func notify(nType uint8, id string, msg string, actions ...Action) *Notification
 
 // Save saves the notification and returns it.
 func (n *Notification) Save() *Notification {
-	notsLock.Lock()
-	defer notsLock.Unlock()
 	n.Lock()
 	defer n.Unlock()
 
+	return n.save(true)
+}
+
+func (n *Notification) save(pushUpdate bool) *Notification {
 	// initialize
 	if n.Created == 0 {
 		n.Created = time.Now().Unix()
@@ -139,39 +141,16 @@ func (n *Notification) Save() *Notification {
 		n.SetKey(fmt.Sprintf("notifications:all/%s", n.ID))
 	}
 
-	// update meta
 	n.UpdateMeta()
 
-	// assign to data map
+	// store the notification inside or map
+	notsLock.Lock()
 	nots[n.ID] = n
+	notsLock.Unlock()
 
-	// push update
+	if pushUpdate {
-	log.Tracef("notifications: pushing update for %s to subscribers", n.Key())
+		log.Tracef("notifications: pushing update for %s to subscribers", n.Key())
-	dbController.PushUpdate(n)
+		dbController.PushUpdate(n)
-
-	// persist
-	if n.Persistent && persistentBasePath != "" {
-		duplicate := &Notification{
-			ID:               n.ID,
-			Message:          n.Message,
-			DataSubject:      n.DataSubject,
-			AvailableActions: duplicateActions(n.AvailableActions),
-			SelectedActionID: n.SelectedActionID,
-			Persistent:       n.Persistent,
-			Created:          n.Created,
-			Expires:          n.Expires,
-			Responded:        n.Responded,
-			Executed:         n.Executed,
-		}
-		duplicate.SetMeta(n.Meta().Duplicate())
-		key := fmt.Sprintf("%s/%s", persistentBasePath, n.ID)
-		duplicate.SetKey(key)
-		go func() {
-			err := dbInterface.Put(duplicate)
-			if err != nil {
-				log.Warningf("notifications: failed to persist notification %s: %s", key, err)
-			}
-		}()
 	}
 
 	return n
@@ -200,17 +179,12 @@ func (n *Notification) Response() <-chan string {
 
 // Update updates/resends a notification if it was not already responded to.
 func (n *Notification) Update(expires int64) {
-	responded := true
 	n.lock.Lock()
-	if n.Responded == 0 {
+	defer n.lock.Unlock()
-		responded = false
-		n.Expires = expires
-	}
-	n.lock.Unlock()
 
-	// save if not yet responded
+	if n.Responded == 0 {
-	if !responded {
+		n.Expires = expires
-		n.Save()
+		n.save(true)
 	}
 }
 
@@ -236,15 +210,6 @@ func (n *Notification) Delete() error {
 	// push update
 	dbController.PushUpdate(n)
 
-	// delete from persistent storage
-	if n.Persistent && persistentBasePath != "" {
-		key := fmt.Sprintf("%s/%s", persistentBasePath, n.ID)
-		err := dbInterface.Delete(key)
-		if err != nil && err != database.ErrNotFound {
-			return fmt.Errorf("failed to delete persisted notification %s from database: %s", key, err)
-		}
-	}
-
 	return nil
 }