From a72acd21b0ef17d6cd9064dd105042b6edb3b8dc Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Thu, 3 Mar 2022 21:41:51 -0700
Subject: [PATCH] core: Retry dynamic config load if config unchanged

(see discussion in #4603)
---
 admin.go |  2 +-
 caddy.go | 47 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/admin.go b/admin.go
index 8558cee1..d31308a7 100644
--- a/admin.go
+++ b/admin.go
@@ -938,7 +938,7 @@ func handleConfig(w http.ResponseWriter, r *http.Request) error {
 		forceReload := r.Header.Get("Cache-Control") == "must-revalidate"
 
 		err := changeConfig(r.Method, r.URL.Path, body, forceReload)
-		if err != nil {
+		if err != nil && !errors.Is(err, errSameConfig) {
 			return err
 		}
 
diff --git a/caddy.go b/caddy.go
index 661d0b0c..02178948 100644
--- a/caddy.go
+++ b/caddy.go
@@ -18,6 +18,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -110,14 +111,19 @@ func Load(cfgJSON []byte, forceReload bool) error {
 		}
 	}()
 
-	return changeConfig(http.MethodPost, "/"+rawConfigKey, cfgJSON, forceReload)
+	err := changeConfig(http.MethodPost, "/"+rawConfigKey, cfgJSON, forceReload)
+	if errors.Is(err, errSameConfig) {
+		err = nil // not really an error
+	}
+	return err
 }
 
 // changeConfig changes the current config (rawCfg) according to the
 // method, traversed via the given path, and uses the given input as
 // the new value (if applicable; i.e. "DELETE" doesn't have an input).
 // If the resulting config is the same as the previous, no reload will
-// occur unless forceReload is true. This function is safe for
+// occur unless forceReload is true. If the config is unchanged and not
+// forcefully reloaded, then errConfigUnchanged This function is safe for
 // concurrent use.
 func changeConfig(method, path string, input []byte, forceReload bool) error {
 	switch method {
@@ -149,7 +155,7 @@ func changeConfig(method, path string, input []byte, forceReload bool) error {
 	// if nothing changed, no need to do a whole reload unless the client forces it
 	if !forceReload && bytes.Equal(rawCfgJSON, newCfg) {
 		Log().Info("config is unchanged")
-		return nil
+		return errSameConfig
 	}
 
 	// find any IDs in this config and index them
@@ -492,39 +498,47 @@ func finishSettingUp(ctx Context, cfg *Config) error {
 			zap.String("module", val.(Module).CaddyModule().ID.Name()),
 			zap.Int("load_delay", int(cfg.Admin.Config.LoadDelay)))
 
-		runLoadedConfig := func(config []byte) {
+		runLoadedConfig := func(config []byte) error {
 			logger.Info("applying dynamically-loaded config")
 			err := changeConfig(http.MethodPost, "/"+rawConfigKey, config, false)
-			if err == nil {
-				logger.Info("successfully applied dynamically-loaded config")
-			} else {
-				logger.Error("failed to run dynamically-loaded config", zap.Error(err))
+			if errors.Is(err, errSameConfig) {
+				return err
 			}
+			if err != nil {
+				logger.Error("failed to run dynamically-loaded config", zap.Error(err))
+				return err
+			}
+			logger.Info("successfully applied dynamically-loaded config")
+			return nil
 		}
 
 		if cfg.Admin.Config.LoadDelay > 0 {
 			go func() {
-				// the loop is here only to iterate if there is an error or a no-op config load,
-				// in which case we simply wait the delay and try again
+				// the loop is here to iterate ONLY if there is an error, a no-op config load,
+				// or an unchanged config; in which case we simply wait the delay and try again
 				for {
 					timer := time.NewTimer(time.Duration(cfg.Admin.Config.LoadDelay))
 					select {
 					case <-timer.C:
 						loadedConfig, err := val.(ConfigLoader).LoadConfig(ctx)
 						if err != nil {
-							Log().Error("failed loading dynamic config; will retry", zap.Error(err))
+							logger.Error("failed loading dynamic config; will retry", zap.Error(err))
 							continue
 						}
 						if loadedConfig == nil {
-							Log().Info("dynamically-loaded config was nil; will retry")
+							logger.Info("dynamically-loaded config was nil; will retry")
+							continue
+						}
+						err = runLoadedConfig(loadedConfig)
+						if errors.Is(err, errSameConfig) {
+							logger.Info("dynamically-loaded config was unchanged; will retry")
 							continue
 						}
-						runLoadedConfig(loadedConfig)
 					case <-ctx.Done():
 						if !timer.Stop() {
 							<-timer.C
 						}
-						Log().Info("stopping dynamic config loading")
+						logger.Info("stopping dynamic config loading")
 					}
 					break
 				}
@@ -798,5 +812,10 @@ var (
 	rawCfgIndex map[string]string
 )
 
+// errSameConfig is returned if the new config is the same
+// as the old one. This isn't usually an actual, actionable
+// error; it's mostly a sentinel value.
+var errSameConfig = errors.New("config is unchanged")
+
 // ImportPath is the package import path for Caddy core.
 const ImportPath = "github.com/caddyserver/caddy/v2"