From ddcd0c34cf6fcddf484ffac016e34e101a83c012 Mon Sep 17 00:00:00 2001
From: aler9 <46489434+aler9@users.noreply.github.com>
Date: Mon, 6 Sep 2021 18:39:15 +0200
Subject: [PATCH] api: fix concurrency issue when creating multiple path
 configs at once (#542)

---
 internal/core/api.go  | 21 ++++++++-------------
 internal/core/core.go | 18 +++++++++---------
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/internal/core/api.go b/internal/core/api.go
index 9d5ea0c7..2c2835ab 100644
--- a/internal/core/api.go
+++ b/internal/core/api.go
@@ -361,10 +361,10 @@ func (a *api) onConfigSet(ctx *gin.Context) {
 	}
 
 	a.mutex.Lock()
+	defer a.mutex.Unlock()
+
 	var newConf conf.Conf
 	cloneStruct(a.conf, &newConf)
-	a.mutex.Unlock()
-
 	fillStruct(&newConf, in)
 
 	err = newConf.CheckAndFillMissing()
@@ -373,9 +373,7 @@ func (a *api) onConfigSet(ctx *gin.Context) {
 		return
 	}
 
-	a.mutex.Lock()
 	a.conf = &newConf
-	a.mutex.Unlock()
 
 	// since reloading the configuration can cause the shutdown of the API,
 	// call it in a goroutine
@@ -394,9 +392,10 @@ func (a *api) onConfigPathsAdd(ctx *gin.Context) {
 	name := ctx.Param("name")
 
 	a.mutex.Lock()
+	defer a.mutex.Unlock()
+
 	var newConf conf.Conf
 	cloneStruct(a.conf, &newConf)
-	a.mutex.Unlock()
 
 	if _, ok := newConf.Paths[name]; ok {
 		ctx.AbortWithStatus(http.StatusBadRequest)
@@ -413,9 +412,7 @@ func (a *api) onConfigPathsAdd(ctx *gin.Context) {
 		return
 	}
 
-	a.mutex.Lock()
 	a.conf = &newConf
-	a.mutex.Unlock()
 
 	// since reloading the configuration can cause the shutdown of the API,
 	// call it in a goroutine
@@ -434,9 +431,10 @@ func (a *api) onConfigPathsEdit(ctx *gin.Context) {
 	name := ctx.Param("name")
 
 	a.mutex.Lock()
+	defer a.mutex.Unlock()
+
 	var newConf conf.Conf
 	cloneStruct(a.conf, &newConf)
-	a.mutex.Unlock()
 
 	newConfPath, ok := newConf.Paths[name]
 	if !ok {
@@ -452,9 +450,7 @@ func (a *api) onConfigPathsEdit(ctx *gin.Context) {
 		return
 	}
 
-	a.mutex.Lock()
 	a.conf = &newConf
-	a.mutex.Unlock()
 
 	// since reloading the configuration can cause the shutdown of the API,
 	// call it in a goroutine
@@ -467,9 +463,10 @@ func (a *api) onConfigPathsDelete(ctx *gin.Context) {
 	name := ctx.Param("name")
 
 	a.mutex.Lock()
+	defer a.mutex.Unlock()
+
 	var newConf conf.Conf
 	cloneStruct(a.conf, &newConf)
-	a.mutex.Unlock()
 
 	if _, ok := newConf.Paths[name]; !ok {
 		ctx.AbortWithStatus(http.StatusBadRequest)
@@ -484,9 +481,7 @@ func (a *api) onConfigPathsDelete(ctx *gin.Context) {
 		return
 	}
 
-	a.mutex.Lock()
 	a.conf = &newConf
-	a.mutex.Unlock()
 
 	// since reloading the configuration can cause the shutdown of the API,
 	// call it in a goroutine
diff --git a/internal/core/core.go b/internal/core/core.go
index a78c8e35..af9221bd 100644
--- a/internal/core/core.go
+++ b/internal/core/core.go
@@ -83,7 +83,7 @@ func New(args []string) (*Core, bool) {
 	err = p.createResources(true)
 	if err != nil {
 		p.Log(logger.Info, "ERR: %s", err)
-		p.closeResources(nil)
+		p.closeResources(nil, false)
 		return nil, false
 	}
 
@@ -91,7 +91,7 @@ func New(args []string) (*Core, bool) {
 		p.confWatcher, err = confwatcher.New(p.confPath)
 		if err != nil {
 			p.Log(logger.Info, "ERR: %s", err)
-			p.closeResources(nil)
+			p.closeResources(nil, false)
 			return nil, false
 		}
 	}
@@ -143,7 +143,7 @@ outer:
 				break outer
 			}
 
-			err = p.reloadConf(newConf)
+			err = p.reloadConf(newConf, false)
 			if err != nil {
 				p.Log(logger.Info, "ERR: %s", err)
 				break outer
@@ -152,7 +152,7 @@ outer:
 		case newConf := <-p.apiConfigSet:
 			p.Log(logger.Info, "reloading configuration (API request)")
 
-			err := p.reloadConf(newConf)
+			err := p.reloadConf(newConf, true)
 			if err != nil {
 				p.Log(logger.Info, "ERR: %s", err)
 				break outer
@@ -165,7 +165,7 @@ outer:
 
 	p.ctxCancel()
 
-	p.closeResources(nil)
+	p.closeResources(nil, false)
 
 	if p.confWatcher != nil {
 		p.confWatcher.Close()
@@ -361,7 +361,7 @@ func (p *Core) createResources(initial bool) error {
 	return nil
 }
 
-func (p *Core) closeResources(newConf *conf.Conf) {
+func (p *Core) closeResources(newConf *conf.Conf, calledByAPI bool) {
 	closeStats := false
 	if newConf == nil {
 		closeStats = true
@@ -489,7 +489,7 @@ func (p *Core) closeResources(newConf *conf.Conf) {
 		if closeAPI {
 			p.api.close()
 			p.api = nil
-		} else {
+		} else if !calledByAPI { // avoid a loop
 			p.api.OnConfReload(newConf)
 		}
 	}
@@ -539,8 +539,8 @@ func (p *Core) closeResources(newConf *conf.Conf) {
 	}
 }
 
-func (p *Core) reloadConf(newConf *conf.Conf) error {
-	p.closeResources(newConf)
+func (p *Core) reloadConf(newConf *conf.Conf, calledByAPI bool) error {
+	p.closeResources(newConf, calledByAPI)
 
 	p.conf = newConf
 	return p.createResources(false)