feat(gateway): add graceful shutdown with signal handling #41
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "sync" | ||
| "sync/atomic" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
| ) | ||
|
|
||
| var ( | ||
| activeRequestsWG sync.WaitGroup | ||
| activeRequestCnt int64 | ||
| ) | ||
|
Comment on lines
+10
to
+13
Contributor
There was a problem hiding this comment. Package-level state prevents test isolation and concurrent execution. The package-level
Consider refactoring to use a struct-based approach: type RequestTracker struct {
wg sync.WaitGroup
cnt int64
}
func NewRequestTracker() *RequestTracker {
return &RequestTracker{}
}
func (rt *RequestTracker) Middleware() gin.HandlerFunc {
return func(c *gin.Context) {
rt.wg.Add(1)
atomic.AddInt64(&rt.cnt, 1)
defer func() {
atomic.AddInt64(&rt.cnt, -1)
rt.wg.Done()
}()
c.Next()
}
}
func (rt *RequestTracker) Wait() {
rt.wg.Wait()
}
func (rt *RequestTracker) Count() int64 {
return atomic.LoadInt64(&rt.cnt)
}This allows each test to create its own isolated
Comment on lines
+10
to
+13
Contributor
There was a problem hiding this comment. Global state with no reset mechanism creates issues in tests and server restarts These global variables are never reset, which causes problems:
Better approach: Either make these per-server instance variables, or provide a reset function for tests. Example: // For tests
func ResetRequestTracking() {
activeRequestsWG = sync.WaitGroup{}
atomic.StoreInt64(&activeRequestCnt, 0)
}Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/request_tracker.go
Line: 10:13
Comment:
**Global state with no reset mechanism creates issues in tests and server restarts**
These global variables are never reset, which causes problems:
1. **Test pollution**: If multiple tests create servers (like in shutdown_test.go), the counters persist between tests, causing incorrect counts
2. **Server restart issues**: If the server is stopped and restarted within the same process, the counts will be wrong
3. **The test in shutdown_test.go only works by accident** because it's the only test, but running multiple shutdown tests would fail
**Better approach**: Either make these per-server instance variables, or provide a reset function for tests. Example:
```go
// For tests
func ResetRequestTracking() {
activeRequestsWG = sync.WaitGroup{}
atomic.StoreInt64(&activeRequestCnt, 0)
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| // TrackInFlightRequests tracks active HTTP requests. | ||
| func TrackInFlightRequests() gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| activeRequestsWG.Add(1) | ||
| atomic.AddInt64(&activeRequestCnt, 1) | ||
|
|
||
| defer func() { | ||
| atomic.AddInt64(&activeRequestCnt, -1) | ||
| activeRequestsWG.Done() | ||
| }() | ||
|
|
||
| c.Next() | ||
| } | ||
| } | ||
|
|
||
| // WaitForInFlightRequests blocks until all active requests finish. | ||
| func WaitForInFlightRequests() { | ||
| activeRequestsWG.Wait() | ||
| } | ||
|
|
||
| // GetActiveRequestCount returns the current number of active requests. | ||
| func GetActiveRequestCount() int64 { | ||
| return atomic.LoadInt64(&activeRequestCnt) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||||||||||||||||||||||||||||||||||||
| package main | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||
| "net/http/httptest" | ||||||||||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| "github.com/gin-gonic/gin" | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| func TestGracefulShutdown_WaitsForInFlightRequests(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||
| gin.SetMode(gin.TestMode) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| r := gin.New() | ||||||||||||||||||||||||||||||||||||||||||
| r.Use(TrackInFlightRequests()) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Simulate slow handler | ||||||||||||||||||||||||||||||||||||||||||
| r.GET("/slow", func(c *gin.Context) { | ||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(200 * time.Millisecond) | ||||||||||||||||||||||||||||||||||||||||||
| c.Status(http.StatusOK) | ||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| srv := &http.Server{ | ||||||||||||||||||||||||||||||||||||||||||
| Handler: r, | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Start test server | ||||||||||||||||||||||||||||||||||||||||||
| ln := httptest.NewUnstartedServer(r) | ||||||||||||||||||||||||||||||||||||||||||
| ln.Config = srv | ||||||||||||||||||||||||||||||||||||||||||
| ln.Start() | ||||||||||||||||||||||||||||||||||||||||||
| defer ln.Close() | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+24
to
+32
Contributor
There was a problem hiding this comment. Test server setup is incorrect. The test mixes
🛠️ Proposed fixUse the httptest server directly without creating a separate - srv := &http.Server{
- Handler: r,
- }
-
// Start test server
- ln := httptest.NewUnstartedServer(r)
- ln.Config = srv
- ln.Start()
- defer ln.Close()
+ ts := httptest.NewServer(r)
+ defer ts.Close()Then update the shutdown logic: - // Shutdown server
- ctx, cancel := context.WithTimeout(context.Background(), time.Second)
- defer cancel()
-
- start := time.Now()
- if err := srv.Shutdown(ctx); err != nil {
- t.Fatalf("shutdown failed: %v", err)
- }
+ // Close the test server (httptest.Server doesn't support graceful shutdown)
+ start := time.Now()
+ ts.Close()Note:
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Make request in background | ||||||||||||||||||||||||||||||||||||||||||
| done := make(chan struct{}) | ||||||||||||||||||||||||||||||||||||||||||
| go func() { | ||||||||||||||||||||||||||||||||||||||||||
| resp, err := http.Get(ln.URL + "/slow") | ||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||
| t.Errorf("request failed: %v", err) | ||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| resp.Body.Close() | ||||||||||||||||||||||||||||||||||||||||||
| close(done) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+43
Contributor
There was a problem hiding this comment. The test should verify that the request completed successfully by checking the response status code, not just that it didn't error. A connection could be closed mid-request and still not return an error, but the response would be incomplete. Add verification:
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/shutdown_test.go
Line: 36:43
Comment:
The test should verify that the request completed successfully by checking the response status code, not just that it didn't error. A connection could be closed mid-request and still not return an error, but the response would be incomplete.
Add verification:
```suggestion
go func() {
resp, err := http.Get(ln.URL + "/slow")
if err != nil {
t.Errorf("request failed: %v", err)
return
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
t.Errorf("expected status 200, got %d", resp.StatusCode)
}
close(done)
}()
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+44
Contributor
There was a problem hiding this comment. Unsafe use of Line 39 calls 🔒 Proposed fixCapture the error in the goroutine and check it in the main test flow: // Make request in background
- done := make(chan struct{})
+ done := make(chan error)
go func() {
resp, err := http.Get(ln.URL + "/slow")
if err != nil {
- t.Errorf("request failed: %v", err)
- return
+ done <- err
+ return
}
resp.Body.Close()
- close(done)
+ done <- nil
}()
// Give request time to start
time.Sleep(50 * time.Millisecond)
// Shutdown server
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
start := time.Now()
if err := srv.Shutdown(ctx); err != nil {
t.Fatalf("shutdown failed: %v", err)
}
WaitForInFlightRequests()
elapsed := time.Since(start)
- <-done
+ if err := <-done; err != nil {
+ t.Fatalf("request failed: %v", err)
+ }
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Give request time to start | ||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(50 * time.Millisecond) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+46
to
+47
Contributor
There was a problem hiding this comment. Sleep-based synchronization creates a race condition. The 50ms sleep (line 47) assumes the background request will have started by then, but this is not guaranteed. The request might:
This makes the test flaky. ⏱️ Proposed fix using proper synchronizationUse a channel to signal when the request has actually started: // Make request in background
- done := make(chan struct{})
+ requestStarted := make(chan struct{})
+ requestDone := make(chan struct{})
+
go func() {
+ // Signal that we're about to make the request
+ close(requestStarted)
resp, err := http.Get(ln.URL + "/slow")
if err != nil {
t.Errorf("request failed: %v", err)
return
}
resp.Body.Close()
- close(done)
+ close(requestDone)
}()
- // Give request time to start
- time.Sleep(50 * time.Millisecond)
+ // Wait for request to actually start
+ <-requestStarted
+ // Give it a moment to enter the handler
+ time.Sleep(10 * time.Millisecond)Or better yet, modify the slow handler to signal when it starts: + handlerStarted := make(chan struct{})
r.GET("/slow", func(c *gin.Context) {
+ close(handlerStarted)
time.Sleep(200 * time.Millisecond)
c.Status(http.StatusOK)
})
// ... later ...
- // Give request time to start
- time.Sleep(50 * time.Millisecond)
+ // Wait for handler to start
+ <-handlerStarted
🤖 Prompt for AI Agents
Comment on lines
+46
to
+47
Contributor
There was a problem hiding this comment. Race condition: Sleep-based synchronization is unreliable Using
Better approach: Use proper synchronization with channels or wait for the request to actually be in-flight. Example: started := make(chan struct{})
go func() {
// Signal when request enters the handler
close(started)
resp, err := http.Get(ln.URL + "/slow")
// ...
}()
<-started // Wait for request to actually startOr check Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/shutdown_test.go
Line: 46:47
Comment:
**Race condition: Sleep-based synchronization is unreliable**
Using `time.Sleep(50 * time.Millisecond)` to "give request time to start" is a classic test race condition:
- On slow systems (CI, loaded machines), 50ms may not be enough and the test will fail
- On fast systems, the request might already be complete by then
- This makes tests flaky and unpredictable
**Better approach**: Use proper synchronization with channels or wait for the request to actually be in-flight. Example:
```go
started := make(chan struct{})
go func() {
// Signal when request enters the handler
close(started)
resp, err := http.Get(ln.URL + "/slow")
// ...
}()
<-started // Wait for request to actually start
```
Or check `GetActiveRequestCount() > 0` in a loop with timeout instead of sleeping.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Shutdown server | ||||||||||||||||||||||||||||||||||||||||||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||||||||||||||||||||||||||||||||||||||||||
| defer cancel() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| start := time.Now() | ||||||||||||||||||||||||||||||||||||||||||
| if err := srv.Shutdown(ctx); err != nil { | ||||||||||||||||||||||||||||||||||||||||||
| t.Fatalf("shutdown failed: %v", err) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| WaitForInFlightRequests() | ||||||||||||||||||||||||||||||||||||||||||
| elapsed := time.Since(start) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+53
to
+59
Contributor
There was a problem hiding this comment. Test doesn't actually verify what it claims to test This test has a fundamental flaw: The test is measuring that What should be tested instead:
Current test: Measures Shutdown() waiting for requests ✓ (not our code) Prompt To Fix With AIThis is a comment left during a code review.
Path: gateway/shutdown_test.go
Line: 53:59
Comment:
**Test doesn't actually verify what it claims to test**
This test has a fundamental flaw: `srv.Shutdown(ctx)` on line 54 already waits for active connections to complete (this is documented behavior of http.Server.Shutdown). So by the time line 58 calls `WaitForInFlightRequests()`, the request has already finished.
The test is measuring that `http.Server.Shutdown()` works (which is a given), not that our middleware tracking works correctly.
**What should be tested instead:**
1. Verify that `GetActiveRequestCount()` returns the correct count WHILE requests are in-flight
2. Test that requests complete successfully (check response status code)
3. Test the interaction between the tracking middleware and actual shutdown
Current test: Measures Shutdown() waiting for requests ✓ (not our code)
Should test: Our tracking middleware correctly counts requests ✗ (not tested)
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| <-done | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Assert shutdown waited for request | ||||||||||||||||||||||||||||||||||||||||||
| if elapsed < 200*time.Millisecond { | ||||||||||||||||||||||||||||||||||||||||||
| t.Fatalf("shutdown did not wait for in-flight request") | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation to use tabs instead of spaces.
The import statements appear to use spaces for indentation instead of tabs. Run
gofmtorgo fmtto ensure consistent formatting with Go standards.🔎 Fix formatting
📝 Committable suggestion
🤖 Prompt for AI Agents