fix(ready): release lock before server shutdown (#8526)

Signed-off-by: Paco Cartones <pacocartones@users.noreply.github.com>
Co-authored-by: Paco Cartones <pacocartones@users.noreply.github.com>

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
 }

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