From 9fb1859b23b5865e9419c8dda1733230f65d3808 Mon Sep 17 00:00:00 2001 From: sam lockart <35046326+alam0rt@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:40:07 +1000 Subject: [PATCH] fix(reload): broadcast shutdown signal (#8504) * fix(reload): broadcast shutdown signal Signed-off-by: alam0rt * fix(reload): scope shutdown state to instances Signed-off-by: alam0rt --------- Signed-off-by: alam0rt --- plugin/reload/reload.go | 47 ++++++++++++++++++-------- plugin/reload/reload_test.go | 65 ++++++++++++++++++++++++++++++++++-- plugin/reload/setup.go | 36 ++++++++++---------- 3 files changed, 115 insertions(+), 33 deletions(-) diff --git a/plugin/reload/reload.go b/plugin/reload/reload.go index af8289d30..839bc5bca 100644 --- a/plugin/reload/reload.go +++ b/plugin/reload/reload.go @@ -22,10 +22,29 @@ const ( ) type reload struct { - dur time.Duration - u int - mtx sync.RWMutex - quit chan bool + dur time.Duration + u int + mtx sync.RWMutex + quit chan struct{} + shutdownOnce sync.Once +} + +func newReload() *reload { + return &reload{dur: defaultInterval, quit: make(chan struct{})} +} + +func (r *reload) shutdown() error { + r.shutdownOnce.Do(func() { + close(r.quit) + }) + return nil +} + +func reloadForInstance(instance *caddy.Instance) *reload { + instance.StorageMu.RLock() + defer instance.StorageMu.RUnlock() + state, _ := instance.Storage[reloadStorageKey{}].(*reload) + return state } func (r *reload) setUsage(u int) { @@ -64,15 +83,13 @@ func hook(event caddy.EventName, info any) error { if event != caddy.InstanceStartupEvent { return nil } - // if reload is removed from the Corefile, then the hook - // is still registered but setup is never called again - // so we need a flag to tell us not to reload - if r.usage() == unused { - return nil - } // this should be an instance. ok to panic if not instance := info.(*caddy.Instance) + r := reloadForInstance(instance) + if r == nil || r.usage() == unused { + return nil + } parsedCorefile, err := parse(instance.Caddyfile()) if err != nil { return err @@ -80,9 +97,11 @@ func hook(event caddy.EventName, info any) error { sha512sum := sha512.Sum512(parsedCorefile) log.Infof("Running configuration SHA512 = %x\n", sha512sum) + quit := r.quit + interval := r.interval() go func() { - tick := time.NewTicker(r.interval()) + tick := time.NewTicker(interval) defer tick.Stop() for { @@ -106,7 +125,7 @@ func hook(event caddy.EventName, info any) error { // change status of usage will be reset in setup if the plugin appears in config file r.setUsage(maybeUsed) // If shutdown is in progress, avoid attempting a restart. - if shutdownRequested(r.quit) { + if shutdownRequested(quit) { return } _, err := instance.Restart(corefile) @@ -122,7 +141,7 @@ func hook(event caddy.EventName, info any) error { } return } - case <-r.quit: + case <-quit: return } } @@ -133,7 +152,7 @@ func hook(event caddy.EventName, info any) error { // shutdownRequested reports whether a shutdown has been requested via quit channel. // helps with unit testing of the shutdown gate logic. -func shutdownRequested(quit <-chan bool) bool { +func shutdownRequested(quit <-chan struct{}) bool { select { case <-quit: return true diff --git a/plugin/reload/reload_test.go b/plugin/reload/reload_test.go index 48cb42137..bef73329b 100644 --- a/plugin/reload/reload_test.go +++ b/plugin/reload/reload_test.go @@ -30,11 +30,11 @@ func TestParseInvalidCorefile(t *testing.T) { func TestShutdownGate(t *testing.T) { t.Parallel() - q := make(chan bool, 1) + q := make(chan struct{}) if shutdownRequested(q) { t.Fatalf("expected no shutdown before signal") } - q <- true + close(q) if !shutdownRequested(q) { t.Fatalf("expected shutdown after signal") } @@ -48,3 +48,64 @@ func TestHookIgnoresNonStartupEvent(t *testing.T) { t.Fatalf("expected no error for non-startup event, got %v", err) } } + +// TestShutdownRequestedBroadcastsClosedSignal ensures a shutdown remains visible to every observer. +func TestShutdownRequestedBroadcastsClosedSignal(t *testing.T) { + quit := make(chan struct{}) + close(quit) + + if !shutdownRequested(quit) { + t.Fatal("expected first shutdownRequested call to observe shutdown") + } + + if !shutdownRequested(quit) { + t.Fatal("expected second shutdownRequested call to observe shutdown as well") + } +} + +// TestSetupCreatesIndependentReloadStates ensures one instance shutdown does not stop another. +func TestSetupCreatesIndependentReloadStates(t *testing.T) { + c1 := caddy.NewTestController("dns", `reload 2s 0s`) + if err := setup(c1); err != nil { + t.Fatalf("expected first setup to succeed, got %v", err) + } + state1, ok := c1.Get(reloadStorageKey{}).(*reload) + if !ok { + t.Fatal("expected first controller to own reload state") + } + + c2 := caddy.NewTestController("dns", `reload 2s 0s`) + if err := setup(c2); err != nil { + t.Fatalf("expected second setup to succeed, got %v", err) + } + state2, ok := c2.Get(reloadStorageKey{}).(*reload) + if !ok { + t.Fatal("expected second controller to own reload state") + } + if state1 == state2 { + t.Fatal("expected each controller to own independent reload state") + } + + if err := state1.shutdown(); err != nil { + t.Fatalf("expected first state shutdown to succeed, got %v", err) + } + if shutdownRequested(state2.quit) { + t.Fatal("expected shutting down the first state not to stop the second") + } +} + +// TestReloadStateShutdownIsIdempotent ensures repeated shutdown callbacks do not panic. +func TestReloadStateShutdownIsIdempotent(t *testing.T) { + state := newReload() + + if err := state.shutdown(); err != nil { + t.Fatalf("expected first shutdown to succeed, got %v", err) + } + if err := state.shutdown(); err != nil { + t.Fatalf("expected second shutdown to succeed, got %v", err) + } + + if !shutdownRequested(state.quit) { + t.Fatal("expected shutdown after state shutdown") + } +} diff --git a/plugin/reload/setup.go b/plugin/reload/setup.go index 47a12e5e4..0cc902624 100644 --- a/plugin/reload/setup.go +++ b/plugin/reload/setup.go @@ -15,14 +15,22 @@ var log = clog.NewWithPlugin("reload") func init() { plugin.Register("reload", setup) } -// the info reload is global to all application, whatever number of reloads. -// it is used to transmit data between Setup and start of the hook called 'onInstanceStartup' -// channel for QUIT is never changed in purpose. -// WARNING: this data may be unsync after an invalid attempt of reload Corefile. -var ( - r = reload{dur: defaultInterval, u: unused, quit: make(chan bool, 1)} - once, shutOnce sync.Once -) +// The event hook is process-global, but reload state belongs to each Caddy instance. +type reloadStorageKey struct{} + +func reloadForController(c *caddy.Controller) *reload { + if state, ok := c.Get(reloadStorageKey{}).(*reload); ok { + return state + } + + state := newReload() + c.Set(reloadStorageKey{}, state) + // Stop this instance's watcher on both reload and final shutdown. + c.OnShutdown(state.shutdown) + return state +} + +var once sync.Once func setup(c *caddy.Controller) error { c.Next() // 'reload' @@ -67,18 +75,12 @@ func setup(c *caddy.Controller) error { } // prepare info for next onInstanceStartup event - r.setInterval(i) - r.setUsage(used) + state := reloadForController(c) + state.setInterval(i) + state.setUsage(used) once.Do(func() { caddy.RegisterEventHook("reload", hook) }) - // re-register on finalShutDown as the instance most-likely will be changed - shutOnce.Do(func() { - c.OnFinalShutdown(func() error { - r.quit <- true - return nil - }) - }) return nil }