From d52dca1cb685845d0cb6d42a0cdc2c17196791e3 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sat, 5 Sep 2026 22:31:11 +0800 Subject: [PATCH] =?UTF-8?q?feat=E2=9C=A8:=20announce=20BeforeRouter=20and?= =?UTF-8?q?=20AfterListen,=20and=20bind=20before=20either?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two phases are now announced from the command that serves traffic, so a module can attach to them instead of being called by name from here. The listener is opened by this goroutine rather than left to ListenAndServe, which binds on the goroutine that serves. That mattered for the phase: a hook on AfterListen is promised a reachable port, and with the bind happening out of sight there was no way to keep that promise - "address already in use" surfaced on a goroutine nobody read, after the banner had already announced the server was up. It is now returned from run() and the process exits non-zero without claiming anything. AfterListen is announced synchronously. Moving it to a goroutine to save the few milliseconds would let it overlap the shutdown, and on a fast SIGTERM the cleanup callbacks could finish before the startup ones did. What is left in the serving goroutine is still log.Fatal, deliberately: the bind is no longer among the errors that reach it, so what remains is a serve that failed after the port was taken, and carrying on would park the process on <-quit with nothing serving. ServeTLS is the one case that can still fail immediately, since it reads the certificate files - with ssl enabled a hook can still run against a server on its way down. That is not a regression (the old code printed the banner in the same situation) and it is not fixed here. BeforeRouter is placed before initRouter, which is a different moment from the before registry runStartupHooks drains: those callbacks run after the engine has been built, not before it. AfterListen is tested here, in one test rather than two because the phase seals itself once it has run: a second test would find a closed registry and pass while proving nothing. Both counter-proofs compile and fail - announcing on a failed bind reports "AfterListen ran 1 times after a failed bind", and `go RunPhase(...)` reports "ran 0 times, want 1" against the hook's own pause. BeforeRouter's placement is not asserted in this commit. The test for it comes with the buildRouter extraction later in this branch. --- cmd/api/phase_test.go | 88 +++++++++++++++++++++++++++++++++++++++++++ cmd/api/server.go | 68 +++++++++++++++++++++++++++------ 2 files changed, 144 insertions(+), 12 deletions(-) create mode 100644 cmd/api/phase_test.go diff --git a/cmd/api/phase_test.go b/cmd/api/phase_test.go new file mode 100644 index 00000000..20d5b80c --- /dev/null +++ b/cmd/api/phase_test.go @@ -0,0 +1,88 @@ +package api + +import ( + "fmt" + "net" + "net/http" + "testing" + "time" + + "github.com/go-admin-team/go-admin-core/v2/sdk" + "github.com/go-admin-team/go-admin-core/v2/sdk/runtime" +) + +// freePort returns a port nothing is listening on. It is inherently a guess - +// the port is free when it is handed back and could be taken a moment later - +// but every alternative needs the caller to hold the listener, which is the one +// thing these tests cannot do. +func freePort(t *testing.T) int { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("probe listen: %v", err) + } + port := ln.Addr().(*net.TCPAddr).Port + _ = ln.Close() + return port +} + +// AfterListen promises a hook that the port is reachable. Both halves of that +// are asserted here, and in one test rather than two, because the phase seals +// itself once it has run: a second test calling RunPhase again would find a +// closed registry and pass while proving nothing. +// +// The failing bind comes first for the same reason. It must leave the phase +// unsealed, which is only visible if nothing has sealed it yet. +func TestAfterListenIsAnnouncedOnlyOnceThePortIsBound(t *testing.T) { + // The pause makes the "announced synchronously" claim testable: if the + // announcement were moved onto a goroutine, startServing would return + // while the hook was still sleeping and the count below would be zero. + var ran int + sdk.Runtime.SetPhase(runtime.AfterListen, func() { + time.Sleep(50 * time.Millisecond) + ran++ + }) + + // Somebody else already has the port. Under ListenAndServe this surfaced + // on the serving goroutine, far too late to stop the announcement. + taken, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("occupy: %v", err) + } + defer func() { _ = taken.Close() }() + + blocked := &http.Server{Addr: taken.Addr().String(), Handler: http.NewServeMux()} + if err := startServing(blocked, false, "", ""); err == nil { + t.Fatal("startServing returned no error for a port that was already taken") + } + if ran != 0 { + t.Errorf("AfterListen ran %d times after a failed bind; a hook there is told the port is reachable", ran) + } + if sdk.Runtime.PhaseSealed(runtime.AfterListen) { + t.Error("a failed bind sealed AfterListen, so the phase could never run for a server that did start") + } + + // And now a bind that works. + port := freePort(t) + srv := &http.Server{Addr: fmt.Sprintf("127.0.0.1:%d", port), Handler: http.NewServeMux()} + if err := startServing(srv, false, "", ""); err != nil { + t.Fatalf("startServing on a free port: %v", err) + } + defer func() { _ = srv.Close() }() + + // Checked the instant startServing returns, so this is also the assertion + // that it did not return early: an asynchronous announcement would still + // be inside the sleep. Synchrony matters because an announcement that + // overlaps the wait below could, on a fast SIGTERM, have the shutdown + // callbacks finish before the startup ones. + if ran != 1 { + t.Fatalf("AfterListen ran %d times, want 1", ran) + } + + // The claim is not "Serve was called" but "the port answers". Dial it. + c, err := net.DialTimeout("tcp", srv.Addr, 5*time.Second) + if err != nil { + t.Fatalf("AfterListen ran but the port does not answer: %v", err) + } + _ = c.Close() +} diff --git a/cmd/api/server.go b/cmd/api/server.go index 646b988b..cdb0cad1 100644 --- a/cmd/api/server.go +++ b/cmd/api/server.go @@ -3,6 +3,7 @@ package api import ( "context" "fmt" + "net" "net/http" "os" "os/signal" @@ -16,6 +17,7 @@ import ( "github.com/go-admin-team/go-admin-core/v2/sdk/api" "github.com/go-admin-team/go-admin-core/v2/sdk/config" "github.com/go-admin-team/go-admin-core/v2/sdk/pkg" + "github.com/go-admin-team/go-admin-core/v2/sdk/runtime" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -81,6 +83,12 @@ func run() error { if config.ApplicationConfig.Mode == pkg.ModeProd.String() { gin.SetMode(gin.ReleaseMode) } + // The last point at which a module can still affect how routes are built. + // It is not the same moment as the before registry, which runStartupHooks + // drains below - those callbacks run after initRouter has built the engine, + // not before it. + sdk.Runtime.RunPhase(runtime.BeforeRouter) + initRouter() runStartupHooks() @@ -122,18 +130,10 @@ func run() error { // arming is separate from waiting. quit, disarmStopSignals := armStopSignals() - go func() { - // 服务连接 - if config.SslConfig.Enable { - if err := srv.ListenAndServeTLS(config.SslConfig.Pem, config.SslConfig.KeyStr); err != nil && !errors.Is(err, http.ErrServerClosed) { - log.Fatal("listen: ", err) - } - } else { - if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { - log.Fatal("listen: ", err) - } - } - }() + if err := startServing(srv, config.SslConfig.Enable, config.SslConfig.Pem, config.SslConfig.KeyStr); err != nil { + return err + } + fmt.Println(pkg.Red(string(global.LogoContent))) tip() fmt.Println(pkg.Green("Server run at:")) @@ -185,6 +185,50 @@ func armStopSignals() (<-chan os.Signal, func()) { return quit, func() { signal.Stop(quit) } } +// startServing binds srv.Addr, hands the listener to srv on its own goroutine, +// and announces AfterListen. +// +// The bind is done here rather than left to ListenAndServe, which binds on the +// goroutine that serves. That put the failure every deployment actually hits - +// "address already in use" - on a goroutine nobody was reading, so the banner +// went on to claim the server was up, and there would be no way to keep +// AfterListen from announcing a socket that does not exist. A hook there is +// promised a reachable port; the only way to keep that promise is for the bind +// to have already happened on this goroutine. +// +// AfterListen is announced synchronously. Running it in a goroutine to save the +// few milliseconds would let it overlap the shutdown: on a fast SIGTERM the +// cleanup callbacks could finish before the startup ones had. +func startServing(srv *http.Server, useTLS bool, pem, key string) error { + ln, err := net.Listen("tcp", srv.Addr) + if err != nil { + return errors.Wrap(err, "listen") + } + + go func() { + // 服务连接 + var err error + if useTLS { + err = srv.ServeTLS(ln, pem, key) + } else { + err = srv.Serve(ln) + } + if err != nil && !errors.Is(err, http.ErrServerClosed) { + // Still fatal, as it was. The bind is no longer among the errors + // that reach here; what is left is a serve that failed after the + // port was taken, and carrying on would park the process on + // <-quit with nothing serving. TLS is the one case that can still + // fail immediately - ServeTLS reads the certificate files - so + // with ssl enabled AfterListen can run against a server that is + // already on its way down. + log.Fatal("listen: ", err) + } + }() + + sdk.Runtime.RunPhase(runtime.AfterListen) + return nil +} + // shutdownServer stops srv, giving in-flight requests up to timeout to finish. // // It returns the error instead of exiting on it. A caller that exits here skips