feat(duplex): add graceful shutdown support and port validation#556
feat(duplex): add graceful shutdown support and port validation#556k4leung4 wants to merge 1 commit into
Conversation
k4leung4
force-pushed
the
kleung/graceful-shutdown
branch
2 times, most recently
from
7270858 to
eb64a24
Compare
Honor context cancellation in ListenAndServe and Serve by shutting down the HTTP server gracefully via http.Server.Shutdown(), allowing in-flight requests (both REST and gRPC-over-HTTP) to complete within a 30-second timeout. Drain the serve error channel and filter http.ErrServerClosed. Note: grpc.Server.GracefulStop() is NOT used because the duplex server runs gRPC via grpcHandlerFunc on an http.Server, which uses serverHandlerTransport — a transport that does not support Drain(). http.Server.Shutdown handles draining for both protocols. Also validate that port is in range [0, 65535] in New(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Kenny Leung <kleung@chainguard.dev>
k4leung4
force-pushed
the
kleung/graceful-shutdown
branch
from
eb64a24 to
b955c1b
Compare
eslerm
left a comment
eslerm
left a comment
There was a problem hiding this comment.
🤖: Graceful shutdown implementation looks clean — serveAndWait with buffered errCh and http.Server.Shutdown is the right approach. Port validation is straightforward. A few suggestions:
| // requests (both REST and gRPC-over-HTTP) to complete. | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) | ||
| defer cancel() | ||
| if err := server.Shutdown(shutdownCtx); err != nil { |
There was a problem hiding this comment.
🤖: When server.Shutdown(shutdownCtx) returns an error (timeout), errCh is never drained — the serve goroutine's error is silently discarded. Not a leak (buffered channel), but if the serve error is meaningful it would be lost. Consider draining errCh before returning the shutdown error.
| }() | ||
|
|
||
| // Give server time to start. | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
🤖: time.Sleep(100ms) as synchronisation — there's no signal from inside slowServer.SayHello confirming the handler has been entered before cancel() fires. On a loaded CI runner, cancel could fire before the RPC is in-flight, making this test validate clean-stop-under-no-load rather than graceful drain. A channel closed at the top of SayHello would make this deterministic.
|
sorry, I think I stole this feature in 24cc91b... can you check if there's anything left over from yours? |
4 participants
Summary
ListenAndServeandServeby callinghttp.Server.Shutdown()when the context is done, allowing in-flight requests (both REST and gRPC-over-HTTP) to complete within a 30-second timeout_ context.Context), meaning in-flight requests were dropped during deploymentssignal.NotifyContext— they will automatically gain graceful shutdown on the next go-grpc-kit bump[0, 65535]inNew()Note:
grpc.Server.GracefulStop()is NOT used because the duplex server runs gRPC viagrpcHandlerFuncon anhttp.Server, which usesserverHandlerTransport— a transport that does not supportDrain().http.Server.Shutdownhandles graceful draining for both REST and gRPC-over-HTTP connections. The serve error channel is drained after shutdown andhttp.ErrServerClosedis filtered as expected.Test plan
go test ./pkg/duplex/ -count=1 -racepassesTestGracefulShutdown— starts a slow in-flight RPC, cancels the server context, verifies the RPC completes successfully (graceful drain) and Serve returns nilTestNewInvalidPort— table-driven test verifying both negative and >65535 ports panic🤖 Generated with Claude Code