httpserver: make /-/reload handler cancellation-safe#5234
Conversation
Fix three bugs in the reload handler identified in prometheus#5103: 1. Use a buffered error channel (size 1) so the reloader goroutine never blocks if the HTTP client disconnects before reading the result. 2. Remove the incorrect `defer close(errc)` — the handler is the receiver, not the sender; closing from the receiver side could panic the sender. 3. Wrap both the enqueue (send to reloadCh) and the reply (receive from errc) in select statements that also listen on req.Context().Done(), so a client disconnect unblocks the handler at either stage. Fixes prometheus#5103 Signed-off-by: June Kim <kimjune01@gmail.com>
📝 WalkthroughWalkthroughThe ChangesReload Handler Cancellation Safety
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant ReloadQueue as reloadCh
participant Reloader
Client->>Handler: POST /-/reload
Handler->>Handler: Create buffered errc chan
alt Context done before send
Handler-->>Client: 422 UnprocessableEntity
else Send errc to queue
Handler->>ReloadQueue: Send errc
ReloadQueue->>Reloader: Receive errc
alt Reloader completes
Reloader->>Handler: Send result on errc
Handler-->>Client: 200 OK (nil) or 500 (error)
else Context done during reload
Client-xHandler: Context cancelled
Handler-->>Client: 422 UnprocessableEntity
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@httpserver/httpserver_test.go`:
- Around line 34-40: The tests spawn goroutines that call require.* (which calls
t.FailNow) — replace these with assert.* or move the assertions out of the
goroutine: capture the httptest.ResponseRecorder (w) and any response values
inside the goroutine, send them on a channel or close(done) and then after
<-done perform require.Equal(t, http.StatusOK, w.Code) in the main test
goroutine; update the four occurrences referencing the done channel,
router.ServeHTTP, httptest.NewRequest and httptest.NewRecorder so assertions run
in the test goroutine or use assert.* inside the goroutine instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40886f45-c83a-456b-a48c-5b6f09629879
📒 Files selected for processing (2)
httpserver/httpserver.gohttpserver/httpserver_test.go
| go func() { | ||
| defer close(done) | ||
| req := httptest.NewRequest("POST", "/-/reload", nil) | ||
| w := httptest.NewRecorder() | ||
| router.ServeHTTP(w, req) | ||
| require.Equal(t, http.StatusOK, w.Code) | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
testify require FailNow from goroutine other than test goroutine
💡 Result:
In Go’s testing and in stretchr/testify, you must not call require.* / t.FailNow (which it uses under the hood) from a goroutine other than the goroutine running the test. Evidence: - Go’s testing.T.FailNow documentation says FailNow “must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.” It calls runtime.Goexit in the current goroutine, so it only stops that goroutine, not the whole test. [1] - testify’s require package documentation states that require “terminate current test” and “must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test.” [2] What to do instead: - Use assert.* inside goroutines (it records an error via t.Errorf-style behavior rather than invoking FailNow). This is the common workaround suggested in testify discussions: “use assert in goroutines, not require.” [3] - Or funnel the result back to the main test goroutine (e.g., via a channel) and perform require/assert in that main goroutine, so FailNow happens on the correct goroutine. [4] Why this matters: - If require calls t.FailNow from the wrong goroutine, runtime.Goexit will exit only that goroutine, so the expected assertion/termination semantics for the test can break (and can even lead to hangs in some patterns). [5][6]
Citations:
- 1: https://go.dev/issue/24678
- 2: https://pkg.go.dev/github.com/stretchr/testify/require
- 3: How to handle failed expectations inside of a goroutine? stretchr/testify#772
- 4: Calling Called() in a goroutine can result in a call to t.FailNow() stretchr/testify#1701
- 5:
Eventuallyhangs on failed/abortedconditionstretchr/testify#1810 - 6: panic: test executed panic(nil) or runtime.Goexit stretchr/testify#573
🏁 Script executed:
# Check the actual test file at the mentioned line ranges
cat -n httpserver/httpserver_test.go | sed -n '30,70p'Repository: prometheus/alertmanager
Length of output: 1246
🏁 Script executed:
# Also check the other two ranges
cat -n httpserver/httpserver_test.go | sed -n '82,125p'Repository: prometheus/alertmanager
Length of output: 1618
Use assert.* or move assertions outside goroutines.
require.* calls t.FailNow(), which per Go's testing documentation must be called from the goroutine running the test—calling it from another goroutine exits only that goroutine via runtime.Goexit(), leaving the test in a misleading state instead of properly failing.
All four new tests have this issue (lines 34–40, 58–65, 86–92, 115–121). Fix by either:
- Moving assertions outside the goroutine and capturing response objects in the main goroutine after
<-done, or - Using
assert.*inside the goroutines to record failures without violating the threading contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@httpserver/httpserver_test.go` around lines 34 - 40, The tests spawn
goroutines that call require.* (which calls t.FailNow) — replace these with
assert.* or move the assertions out of the goroutine: capture the
httptest.ResponseRecorder (w) and any response values inside the goroutine, send
them on a channel or close(done) and then after <-done perform require.Equal(t,
http.StatusOK, w.Code) in the main test goroutine; update the four occurrences
referencing the done channel, router.ServeHTTP, httptest.NewRequest and
httptest.NewRecorder so assertions run in the test goroutine or use assert.*
inside the goroutine instead.
1 participant
Summary
/-/reloadHTTP handler where a cancelled client request could leave the reload channel in a stuck stateselectwithcontext.Done()to detect client disconnection before sending on the reload channelFixes #5103
Test plan
Summary by CodeRabbit
Bug Fixes
Tests