fix(reload): broadcast shutdown signal (#8504)

* fix(reload): broadcast shutdown signal

Signed-off-by: alam0rt <sam@samlockart.com>

* fix(reload): scope shutdown state to instances

Signed-off-by: alam0rt <sam@samlockart.com>

---------

Signed-off-by: alam0rt <sam@samlockart.com>

plugin/reload/reload.go

@@ -22,10 +22,29 @@ const (
 )
 
 type reload struct {
-	dur  time.Duration
+	dur          time.Duration
-	u    int
+	u            int
-	mtx  sync.RWMutex
+	mtx          sync.RWMutex
-	quit chan bool
+	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

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")
+	}
+}

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.
+// The event hook is process-global, but reload state belongs to each Caddy instance.
-// it is used to transmit data between Setup and start of the hook called 'onInstanceStartup'
+type reloadStorageKey struct{}
-// channel for QUIT is never changed in purpose.
+
-// WARNING: this data may be unsync after an invalid attempt of reload Corefile.
+func reloadForController(c *caddy.Controller) *reload {
-var (
+	if state, ok := c.Get(reloadStorageKey{}).(*reload); ok {
-	r              = reload{dur: defaultInterval, u: unused, quit: make(chan bool, 1)}
+		return state
-	once, shutOnce sync.Once
+	}
-)
+
+	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)
+	state := reloadForController(c)
-	r.setUsage(used)
+	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
 }