From b51dc5d5d0b8764165170af1f54b77d6de8cb5a1 Mon Sep 17 00:00:00 2001
From: Matt Holt <mholt@users.noreply.github.com>
Date: Fri, 21 Jul 2023 15:32:20 -0600
Subject: core: Refine mutex during reloads (fix #5628) (#5645)

Separate currentCtxMu to protect currentCtx, and a new
rawCfgMu to protect rawCfg and synchronize loads.
---
 caddy.go | 52 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 18 deletions(-)

(limited to 'caddy.go')

diff --git a/caddy.go b/caddy.go
index dcaa86b..76d51c8 100644
--- a/caddy.go
+++ b/caddy.go
@@ -156,8 +156,8 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 		return fmt.Errorf("method not allowed")
 	}
 
-	currentCtxMu.Lock()
-	defer currentCtxMu.Unlock()
+	rawCfgMu.Lock()
+	defer rawCfgMu.Unlock()
 
 	if ifMatchHeader != "" {
 		// expect the first and last character to be quotes
@@ -257,8 +257,8 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 // readConfig traverses the current config to path
 // and writes its JSON encoding to out.
 func readConfig(path string, out io.Writer) error {
-	currentCtxMu.RLock()
-	defer currentCtxMu.RUnlock()
+	rawCfgMu.RLock()
+	defer rawCfgMu.RUnlock()
 	return unsyncedConfigAccess(http.MethodGet, path, nil, out)
 }
 
@@ -305,7 +305,7 @@ func indexConfigObjects(ptr any, configPath string, index map[string]string) err
 // it as the new config, replacing any other current config.
 // It does NOT update the raw config state, as this is a
 // lower-level function; most callers will want to use Load
-// instead. A write lock on currentCtxMu is required! If
+// instead. A write lock on rawCfgMu is required! If
 // allowPersist is false, it will not be persisted to disk,
 // even if it is configured to.
 func unsyncedDecodeAndRun(cfgJSON []byte, allowPersist bool) error {
@@ -340,8 +340,10 @@ func unsyncedDecodeAndRun(cfgJSON []byte, allowPersist bool) error {
 	}
 
 	// swap old context (including its config) with the new one
+	currentCtxMu.Lock()
 	oldCtx := currentCtx
 	currentCtx = ctx
+	currentCtxMu.Unlock()
 
 	// Stop, Cleanup each old app
 	unsyncedStop(oldCtx)
@@ -627,22 +629,35 @@ type ConfigLoader interface {
 // stop the others. Stop should only be called
 // if not replacing with a new config.
 func Stop() error {
+	currentCtxMu.RLock()
+	ctx := currentCtx
+	currentCtxMu.RUnlock()
+
+	rawCfgMu.Lock()
+	unsyncedStop(ctx)
+
 	currentCtxMu.Lock()
-	defer currentCtxMu.Unlock()
-	unsyncedStop(currentCtx)
 	currentCtx = Context{}
+	currentCtxMu.Unlock()
+
 	rawCfgJSON = nil
 	rawCfgIndex = nil
 	rawCfg[rawConfigKey] = nil
+	rawCfgMu.Unlock()
+
 	return nil
 }
 
-// unsyncedStop stops cfg from running, but has
-// no locking around cfg. It is a no-op if cfg is
-// nil. If any app returns an error when stopping,
+// unsyncedStop stops ctx from running, but has
+// no locking around ctx. It is a no-op if ctx has a
+// nil cfg. If any app returns an error when stopping,
 // it is logged and the function continues stopping
 // the next app. This function assumes all apps in
-// cfg were successfully started first.
+// ctx were successfully started first.
+//
+// A lock on rawCfgMu is required, even though this
+// function does not access rawCfg, that lock
+// synchronizes the stop/start of apps.
 func unsyncedStop(ctx Context) {
 	if ctx.cfg == nil {
 		return
@@ -959,9 +974,8 @@ func Version() (simple, full string) {
 // This function is experimental and might be changed
 // or removed in the future.
 func ActiveContext() Context {
-	// TODO: This locking might still be needed; more investigation is required (deadlock during Cleanup for the caddytls.TLS module).
-	// currentCtxMu.RLock()
-	// defer currentCtxMu.RUnlock()
+	currentCtxMu.RLock()
+	defer currentCtxMu.RUnlock()
 	return currentCtx
 }
 
@@ -970,14 +984,12 @@ type CtxKey string
 
 // This group of variables pertains to the current configuration.
 var (
-	// currentCtxMu protects everything in this var block.
-	currentCtxMu sync.RWMutex
-
 	// currentCtx is the root context for the currently-running
 	// configuration, which can be accessed through this value.
 	// If the Config contained in this value is not nil, then
 	// a config is currently active/running.
-	currentCtx Context
+	currentCtx   Context
+	currentCtxMu sync.RWMutex
 
 	// rawCfg is the current, generic-decoded configuration;
 	// we initialize it as a map with one field ("config")
@@ -995,6 +1007,10 @@ var (
 	// rawCfgIndex is the map of user-assigned ID to expanded
 	// path, for converting /id/ paths to /config/ paths.
 	rawCfgIndex map[string]string
+
+	// rawCfgMu protects all the rawCfg fields and also
+	// essentially synchronizes config changes/reloads.
+	rawCfgMu sync.RWMutex
 )
 
 // errSameConfig is returned if the new config is the same
-- 
cgit v1.2.3