plugin/secondary: stop update loop on reload shutdown (#8198)

plugin/file/secondary.go

@@ -116,11 +116,16 @@ func less(a, b uint32) bool {
 func (z *Zone) Update(updateShutdown chan bool, t *transfer.Transfer) error {
 	// If we don't have a SOA, we don't have a zone, wait for it to appear.
 	for z.getSOA() == nil {
-		time.Sleep(1 * time.Second)
+		if waitOrShutdown(updateShutdown, time.Second) {
+			return nil
+		}
 	}
 	retryActive := false
 
 Restart:
+	if updateStopped(updateShutdown) {
+		return nil
+	}
 	soa := z.getSOA()
 	refresh := time.Second * time.Duration(soa.Refresh)
 	retry := time.Second * time.Duration(soa.Retry)
@@ -145,7 +150,10 @@ Restart:
 				break
 			}
 
-			time.Sleep(jitter(2000)) // 2s randomize
+			if waitOrShutdown(updateShutdown, jitter(2000)) { // 2s randomize
+				stopUpdateTickers(refreshTicker, retryTicker, expireTicker)
+				return nil
+			}
 
 			ok, err := z.shouldTransfer()
 			if err != nil {
@@ -162,14 +170,15 @@ Restart:
 
 			// no errors, stop timers and restart
 			retryActive = false
-			refreshTicker.Stop()
+			stopUpdateTickers(refreshTicker, retryTicker, expireTicker)
-			retryTicker.Stop()
-			expireTicker.Stop()
 			goto Restart
 
 		case <-refreshTicker.C:
 
-			time.Sleep(jitter(5000)) // 5s randomize
+			if waitOrShutdown(updateShutdown, jitter(5000)) { // 5s randomize
+				stopUpdateTickers(refreshTicker, retryTicker, expireTicker)
+				return nil
+			}
 
 			ok, err := z.shouldTransfer()
 			if err != nil {
@@ -188,20 +197,45 @@ Restart:
 
 			// no errors, stop timers and restart
 			retryActive = false
-			refreshTicker.Stop()
+			stopUpdateTickers(refreshTicker, retryTicker, expireTicker)
-			retryTicker.Stop()
-			expireTicker.Stop()
 			goto Restart
 
 		case <-updateShutdown:
-			refreshTicker.Stop()
+			stopUpdateTickers(refreshTicker, retryTicker, expireTicker)
-			retryTicker.Stop()
-			expireTicker.Stop()
 			return nil
 		}
 	}
 }
 
+func waitOrShutdown(updateShutdown <-chan bool, d time.Duration) bool {
+	if updateStopped(updateShutdown) {
+		return true
+	}
+	timer := time.NewTimer(d)
+	defer timer.Stop()
+	select {
+	case <-timer.C:
+		return false
+	case <-updateShutdown:
+		return true
+	}
+}
+
+func updateStopped(updateShutdown <-chan bool) bool {
+	select {
+	case <-updateShutdown:
+		return true
+	default:
+		return false
+	}
+}
+
+func stopUpdateTickers(tickers ...*time.Ticker) {
+	for _, ticker := range tickers {
+		ticker.Stop()
+	}
+}
+
 // jitter returns a random duration between [0,n) * time.Millisecond
 func jitter(n int) time.Duration {
 	r := rand.Intn(n) // #nosec G404 -- non-cryptographic jitter to spread transfer attempts.

plugin/file/secondary_test.go

@@ -3,6 +3,7 @@ package file
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/test"
@@ -121,6 +122,27 @@ func TestTransferIn(t *testing.T) {
 	}
 }
 
+func TestUpdateStopsBeforeInitialTransfer(t *testing.T) {
+	z := NewZone(testZone, "test")
+	updateShutdown := make(chan bool)
+	done := make(chan struct{})
+
+	go func() {
+		if err := z.Update(updateShutdown, nil); err != nil {
+			t.Errorf("Unexpected update error: %v", err)
+		}
+		close(done)
+	}()
+
+	close(updateShutdown)
+
+	select {
+	case <-done:
+	case <-time.After(200 * time.Millisecond):
+		t.Fatal("Update did not stop while waiting for initial SOA")
+	}
+}
+
 func TestIsNotify(t *testing.T) {
 	z := new(Zone)
 	z.origin = testZone

plugin/secondary/setup.go

@@ -1,6 +1,7 @@
 package secondary
 
 import (
+	"sync"
 	"time"
 
 	"github.com/coredns/caddy"
@@ -42,6 +43,7 @@ func setup(c *caddy.Controller) error {
 		if len(z.TransferFrom) > 0 {
 			// In order to support secondary plugin reloading.
 			updateShutdown := make(chan bool)
+			var updateShutdownOnce sync.Once
 
 			c.OnStartup(func() error {
 				z.StartupOnce.Do(func() {
@@ -54,24 +56,26 @@ func setup(c *caddy.Controller) error {
 								break
 							}
 							log.Warningf("All '%s' masters failed to transfer, retrying in %s: %s", n, dur.String(), err)
-							time.Sleep(dur)
+							if waitForTransferRetry(updateShutdown, dur) {
+								return
+							}
 							dur <<= 1 // double the duration
 							if dur > max {
 								dur = max
 							}
+						}
 						select {
 						case <-updateShutdown:
 							return
 						default:
 						}
-						}
 						z.Update(updateShutdown, x)
 					}()
 				})
 				return nil
 			})
 			c.OnShutdown(func() error {
-				updateShutdown <- true
+				updateShutdownOnce.Do(func() { close(updateShutdown) })
 				return nil
 			})
 		}
@@ -85,6 +89,17 @@ func setup(c *caddy.Controller) error {
 	return nil
 }
 
+func waitForTransferRetry(updateShutdown <-chan bool, dur time.Duration) bool {
+	timer := time.NewTimer(dur)
+	defer timer.Stop()
+	select {
+	case <-timer.C:
+		return false
+	case <-updateShutdown:
+		return true
+	}
+}
+
 func secondaryParse(c *caddy.Controller) (file.Zones, fall.F, error) {
 	z := make(map[string]*file.Zone)
 	names := []string{}