summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatt Holt <mholt@users.noreply.github.com>2023-07-21 15:32:20 -0600
committerGitHub <noreply@github.com>2023-07-21 15:32:20 -0600
commitb51dc5d5d0b8764165170af1f54b77d6de8cb5a1 (patch)
treed94262de1f318855c1cb723dd84ff4de3e96b6c4
parentf857b32d65de882dbc945c94029849a7fc895e36 (diff)
core: Refine mutex during reloads (fix #5628) (#5645)
Separate currentCtxMu to protect currentCtx, and a new rawCfgMu to protect rawCfg and synchronize loads.
-rw-r--r--admin.go4
-rw-r--r--caddy.go52
-rw-r--r--context.go6
3 files changed, 42 insertions, 20 deletions
diff --git a/admin.go b/admin.go
index 3ea5051..4a1d23b 100644
--- a/admin.go
+++ b/admin.go
@@ -1016,9 +1016,9 @@ func handleConfigID(w http.ResponseWriter, r *http.Request) error {
id := parts[2]
// map the ID to the expanded path
- currentCtxMu.RLock()
+ rawCfgMu.RLock()
expanded, ok := rawCfgIndex[id]
- currentCtxMu.RUnlock()
+ rawCfgMu.RUnlock()
if !ok {
return APIError{
HTTPStatus: http.StatusNotFound,
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
diff --git a/context.go b/context.go
index 004dee6..85978d4 100644
--- a/context.go
+++ b/context.go
@@ -441,6 +441,12 @@ func (ctx Context) App(name string) (any, error) {
// or stop App modules. The caller is expected to assert to the
// concrete type.
func (ctx Context) AppIfConfigured(name string) any {
+ if ctx.cfg == nil {
+ // this can happen if the currently-active context
+ // is being accessed, but no config has successfully
+ // been loaded yet
+ return nil
+ }
return ctx.cfg.apps[name]
}