diff --git a/plugin/ready/ready.go b/plugin/ready/ready.go index 59b613762..b0a1e1882 100644 --- a/plugin/ready/ready.go +++ b/plugin/ready/ready.go @@ -80,10 +80,12 @@ func (rd *ready) onStartup() error { func (rd *ready) onFinalShutdown() error { rd.Lock() - defer rd.Unlock() if !rd.done { + rd.Unlock() return nil } + rd.done = false + rd.Unlock() uniqAddr.Unset(rd.Addr) @@ -92,6 +94,5 @@ func (rd *ready) onFinalShutdown() error { if err := rd.srv.Shutdown(ctx); err != nil { log.Infof("Failed to stop ready http server: %s", err) } - rd.done = false return nil } diff --git a/plugin/ready/ready_test.go b/plugin/ready/ready_test.go index 561865375..0920075fa 100644 --- a/plugin/ready/ready_test.go +++ b/plugin/ready/ready_test.go @@ -3,8 +3,11 @@ package ready import ( "context" "fmt" + "io" + "net" "net/http" "testing" + "time" "github.com/coredns/coredns/plugin/erratic" clog "github.com/coredns/coredns/plugin/pkg/log" @@ -121,3 +124,80 @@ func TestReady_Continuously(t *testing.T) { } response.Body.Close() } + +func TestReadyShutdownDoesNotHoldLockWhileWaitingForHandlers(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + + accepted := make(chan struct{}) + proceed := make(chan struct{}) + shutdownStarted := make(chan struct{}) + rd := &ready{Addr: ln.Addr().String(), done: true, ln: ln} + rd.srv = &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + close(accepted) + <-proceed + rd.Lock() + defer rd.Unlock() + if !rd.done { + w.WriteHeader(http.StatusServiceUnavailable) + io.WriteString(w, "Shutting down") + return + } + w.WriteHeader(http.StatusOK) + })} + rd.srv.RegisterOnShutdown(func() { close(shutdownStarted) }) + go rd.srv.Serve(ln) + + response := make(chan *http.Response, 1) + requestErr := make(chan error, 1) + go func() { + res, err := http.Get("http://" + ln.Addr().String()) + if err != nil { + requestErr <- err + return + } + response <- res + }() + select { + case <-accepted: + case err := <-requestErr: + t.Fatalf("readiness request failed before reaching handler: %v", err) + case <-time.After(time.Second): + t.Fatal("readiness request did not reach handler") + } + + shutdownDone := make(chan error, 1) + go func() { shutdownDone <- rd.onFinalShutdown() }() + select { + case <-shutdownStarted: + case err := <-shutdownDone: + t.Fatalf("shutdown returned before reaching server: %v", err) + case <-time.After(time.Second): + t.Fatal("ready server shutdown did not start") + } + close(proceed) + + select { + case err := <-requestErr: + t.Fatalf("readiness request failed: %v", err) + case res := <-response: + defer res.Body.Close() + if res.StatusCode != http.StatusServiceUnavailable { + t.Fatalf("expected shutdown response %d, got %d", http.StatusServiceUnavailable, res.StatusCode) + } + case <-time.After(time.Second): + t.Fatal("readiness request blocked behind shutdown") + } + + select { + case err := <-shutdownDone: + if err != nil { + t.Fatalf("shutdown failed: %v", err) + } + case <-time.After(time.Second): + t.Fatal("ready server shutdown blocked waiting for its handler") + } +}