From e50553951fc696c82bcc71410356dc9e9d66150b Mon Sep 17 00:00:00 2001
From: Daniel <dhaavi@users.noreply.github.com>
Date: Mon, 18 Mar 2019 16:30:17 +0100
Subject: [PATCH] Fix notification locking issues

---
 notifications/database.go     | 32 +++++++++++++++++++++-----------
 notifications/notification.go | 30 +++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/notifications/database.go b/notifications/database.go
index 6e8fcfe..e09bb76 100644
--- a/notifications/database.go
+++ b/notifications/database.go
@@ -11,6 +11,7 @@ import (
 	"github.com/Safing/portbase/database/query"
 	"github.com/Safing/portbase/database/record"
 	"github.com/Safing/portbase/database/storage"
+	"github.com/Safing/portbase/log"
 )
 
 var (
@@ -44,8 +45,8 @@ type StorageInterface struct {
 
 // Get returns a database record.
 func (s *StorageInterface) Get(key string) (record.Record, error) {
-	notsLock.Lock()
-	defer notsLock.Unlock()
+	notsLock.RLock()
+	defer notsLock.RUnlock()
 
 	// transform key
 	if strings.HasPrefix(key, "all/") {
@@ -72,8 +73,8 @@ func (s *StorageInterface) Query(q *query.Query, local, internal bool) (*iterato
 }
 
 func (s *StorageInterface) processQuery(q *query.Query, it *iterator.Iterator) {
-	notsLock.Lock()
-	defer notsLock.Unlock()
+	notsLock.RLock()
+	defer notsLock.RUnlock()
 
 	// send all notifications
 	for _, n := range nots {
@@ -107,10 +108,9 @@ func registerAsDatabase() error {
 
 // Put stores a record in the database.
 func (s *StorageInterface) Put(r record.Record) error {
-	r.Lock()
+	// record is already locked!
 	key := r.DatabaseKey()
 	n, err := EnsureNotification(r)
-	r.Unlock()
 
 	if err != nil {
 		return ErrInvalidData
@@ -123,22 +123,32 @@ func (s *StorageInterface) Put(r record.Record) error {
 		return ErrInvalidPath
 	}
 
-	notsLock.Lock()
+	// continue in goroutine
+	go updateNotificationFromDatabasePut(n, key)
+
+	return nil
+}
+
+func updateNotificationFromDatabasePut(n *Notification, key string) {
+	// seperate goroutine in order to correctly lock notsLock
+	notsLock.RLock()
 	origN, ok := nots[key]
-	notsLock.Unlock()
+	notsLock.RUnlock()
 
 	if ok {
+		// existing notification, update selected action ID only
 		n.Lock()
 		defer n.Unlock()
-		go origN.SelectAndExecuteAction(n.SelectedActionID)
+		if n.SelectedActionID != "" {
+			log.Tracef("notifications: user selected action for %s: %s", n.ID, n.SelectedActionID)
+			go origN.SelectAndExecuteAction(n.SelectedActionID)
+		}
 	} else {
 		// accept new notification as is
 		notsLock.Lock()
 		nots[key] = n
 		notsLock.Unlock()
 	}
-
-	return nil
 }
 
 // Delete deletes a record from the database.
diff --git a/notifications/notification.go b/notifications/notification.go
index 005d427..4af05f8 100644
--- a/notifications/notification.go
+++ b/notifications/notification.go
@@ -53,8 +53,8 @@ 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.Lock()
-	defer notsLock.Unlock()
+	notsLock.RLock()
+	defer notsLock.RUnlock()
 	n, ok := nots[id]
 	if ok {
 		return n
@@ -62,13 +62,13 @@ func Get(id string) *Notification {
 	return nil
 }
 
-// New returns a new Notification
+// Init initializes a Notification and returns it.
 func (n *Notification) Init() *Notification {
 	n.Created = time.Now().Unix()
 	return n
 }
 
-// Save saves the notification
+// Save saves the notification and returns it.
 func (n *Notification) Save() *Notification {
 	notsLock.Lock()
 	defer notsLock.Unlock()
@@ -119,16 +119,16 @@ func (n *Notification) Save() *Notification {
 // SetActionFunction sets a trigger function to be executed when the user reacted on the notification.
 // The provided funtion 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 {
-	n.Lock()
-	defer n.Unlock()
+	n.lock.Lock()
+	defer n.lock.Unlock()
 	n.actionFunction = fn
 	return n
 }
 
 // MakeAck sets a default "OK" action and a no-op action function.
 func (n *Notification) MakeAck() *Notification {
-	n.Lock()
-	defer n.Unlock()
+	n.lock.Lock()
+	defer n.lock.Unlock()
 
 	n.AvailableActions = []*Action{
 		&Action{
@@ -144,8 +144,8 @@ func (n *Notification) MakeAck() *Notification {
 
 // Response waits for the user to respond to the notification and returns the selected action.
 func (n *Notification) Response() <-chan string {
-	n.Lock()
-	defer n.Unlock()
+	n.lock.Lock()
+	defer n.lock.Unlock()
 
 	if n.actionTrigger == nil {
 		n.actionTrigger = make(chan string)
@@ -190,12 +190,13 @@ func (n *Notification) SelectAndExecuteAction(id string) {
 	}
 	if n.actionTrigger != nil {
 		// satisfy all listeners
+	triggerAll:
 		for {
 			select {
 			case n.actionTrigger <- n.SelectedActionID:
 				executed = true
 			default:
-				break
+				break triggerAll
 			}
 		}
 	}
@@ -208,6 +209,13 @@ func (n *Notification) SelectAndExecuteAction(id string) {
 	go n.Save()
 }
 
+// AddDataSubject adds the data subject to the notification. This is the only way how a data subject should be added - it avoids locking problems.
+func (n *Notification) AddDataSubject(ds sync.Locker) {
+	n.lock.Lock()
+	defer n.lock.Unlock()
+	n.DataSubject = ds
+}
+
 // Lock locks the Notification and the DataSubject, if available.
 func (n *Notification) Lock() {
 	n.lock.Lock()