From 799e892a68a60c8db4a9a469b3fdaa8f2eb2bfad Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sun, 6 Sep 2026 21:31:08 +0800 Subject: [PATCH] =?UTF-8?q?refactor=E2=99=BB=EF=B8=8F:=20run=20the=20shutd?= =?UTF-8?q?own=20sequence=20from=20one=20function?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The steps between the stop signal and the last log line were written inline in run(), which left nothing for a test to call. The signal tests reproduce that sequence instead: they build their own server, restore their own disposition, and call shutdownServer and runShutdownHooks in an order of their own. So they assert against a copy - reorder the real sequence, or drop a step from it, and they stay green. The sequence now lives in gracefulShutdown, and the waits it spends are a budget rather than two constants read at the point of use. Nothing changes about what happens or in what order: the same disposition is restored first, the same two announcements are made, the same waits are spent, and run() logs the same two errors with the same messages. Returning those errors instead of logging them inside is what lets a caller other than run() react to them. That matters for the next commit, where the tests stop reproducing this sequence and start running it. --- cmd/api/server.go | 61 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/cmd/api/server.go b/cmd/api/server.go index 9c77c1d8..375d0ff8 100644 --- a/cmd/api/server.go +++ b/cmd/api/server.go @@ -209,9 +209,50 @@ func run() error { fmt.Printf("%s Enter Control + C Shutdown Server \r\n", pkg.GetCurrentTimeStr()) <-quit + serverErr, cleanupErr := gracefulShutdown(srv, disarmStopSignals, defaultBudget()) + if serverErr != nil { + // Not log.Fatal: that is an unconditional os.Exit(1), and Shutdown + // reports an error exactly when connections were still in flight - + // which is when the cleanup that ran after it mattered most. + log.Error("Server Shutdown: ", serverErr) + } + if cleanupErr != nil { + log.Error("Cleanup: ", cleanupErr) + } + + return nil +} + +// budget is the waits a shutdown spends, in the order it spends them. +type budget struct { + server time.Duration + cleanup time.Duration +} + +// defaultBudget is what a shutdown spends today. +func defaultBudget() budget { + return budget{server: shutdownTimeout, cleanup: cleanupTimeout} +} + +// gracefulShutdown takes the process down in the order that gives something +// else a chance to notice first. +// +// The whole order lives here so that it has one home. run() is not meant to be +// the only caller: a test that reproduces this sequence instead of running it +// asserts against its own copy, and stays green while the sequence it was +// written for regresses. +// +// The caller has already taken the stop signal off its channel. disarm is +// called first, before anything is taken apart: from that point a second +// signal reaches the default handler again, so a shutdown that hangs can still +// be interrupted. +// +// The two waits' errors are returned separately rather than logged: they fail +// for different reasons, and the caller decides what each is worth. +func gracefulShutdown(srv *http.Server, disarm func(), b budget) (serverErr, cleanupErr error) { // Restored here, not deferred: from this point a second signal must reach // the default handler, so a shutdown that hangs can still be interrupted. - disarmStopSignals() + disarm() // Said before anything is taken apart. A configuration reload arriving in // this window would otherwise re-run AfterResource - rebuilding the pool @@ -226,20 +267,14 @@ func run() error { health.BeginDraining() log.Info("Shutdown Server ... ") - if err := shutdownServer(srv, shutdownTimeout); err != nil { - // Not log.Fatal: that is an unconditional os.Exit(1), and Shutdown - // reports an error exactly when connections were still in flight - - // which is when the cleanup that follows matters most. - log.Error("Server Shutdown: ", err) - } - - // Runs whether or not the line above reported an error, for that reason. - if err := runShutdownHooks(cleanupTimeout); err != nil { - log.Error("Cleanup: ", err) - } + serverErr = shutdownServer(srv, b.server) + // Runs whether or not the wait above failed, and deliberately so: Shutdown + // reports an error exactly when connections were still in flight, which is + // when there is most left to clean up after. + cleanupErr = runShutdownHooks(b.cleanup) log.Info("Server exiting") - return nil + return serverErr, cleanupErr } // shutdownTimeout is how long Shutdown waits for in-flight requests, and