From b51dc5d5d0b8764165170af1f54b77d6de8cb5a1 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Fri, 21 Jul 2023 15:32:20 -0600 Subject: [PATCH] core: Refine mutex during reloads (fix #5628) (#5645) Separate currentCtxMu to protect currentCtx, and a new rawCfgMu to protect rawCfg and synchronize loads. --- admin.go | 4 ++-- caddy.go | 52 ++++++++++++++++++++++++++++++++++------------------ context.go | 6 ++++++ 3 files changed, 42 insertions(+), 20 deletions(-) diff --git a/admin.go b/admin.go index 3ea5051a..4a1d23b6 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 dcaa86b0..76d51c83 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 004dee60..85978d49 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] }