fix: add HTTP client timeouts and improve error handling in publish/subscribe/list#99
fix: add HTTP client timeouts and improve error handling in publish/subscribe/list#99shedybaba wants to merge 1 commit intogetoptimum:mainfrom
Conversation
…ubscribe/list Replace http.DefaultClient (zero timeout) with a shared httpClient configured with a 30-second timeout across publish, subscribe, and list-topics commands. This prevents the CLI from hanging indefinitely when the remote server is unreachable or slow to respond. Other commands in the codebase (health, tracer, update) already use properly configured HTTP clients — this aligns the remaining commands with the established pattern. Additional fixes in the same paths: - Use bytes.NewReader instead of strings.NewReader(string(...)) in publish to avoid an unnecessary heap allocation on every request - Wrap the bare error return in publish HTTP request creation with descriptive context (consistent with all other error returns) - Check io.ReadAll errors in publish and subscribe instead of silently discarding them, which previously produced empty error messages
📝 WalkthroughWalkthroughReplaced the default HTTP client with a configured httpClient instance featuring a 30-second timeout across four command files. The new client is initialized in utils.go and used in list.go, publish.go, and subscribe.go. Enhanced error handling for HTTP response body reading in publish.go and subscribe.go. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/subscribe.go (1)
159-160:⚠️ Potential issue | 🟠 MajorDelay signal registration or bind the request to a cancelable context.
signal.Notifyis armed before the blocking HTTP subscribe request, butsigChanis not consumed until after the request succeeds. If the server is slow/unreachable and the user presses Ctrl+C during Line 229, the signal is intercepted and the command sits until the 30s HTTP timeout expires. Move thesignal.Notifysetup below the subscribe call, or create the request with a signal-aware context so Ctrl+C aborts the POST immediately.Also applies to: 219-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/subscribe.go` around lines 159 - 160, The signal handling is registered before the blocking HTTP subscribe POST and the sigChan isn't consumed until after the request, so Ctrl+C won't abort the request; fix by creating the HTTP request with a cancelable context and wiring signals to cancel it (use context.WithCancel or signal.NotifyContext) so the in-flight subscribe POST can be aborted on SIGINT/SIGTERM; specifically update the code around sigChan/signal.Notify and the subscribe HTTP request (use http.NewRequestWithContext or pass the context into the function that performs the subscribe call) so the request listens to the cancel context rather than relying on a later-read sigChan, or alternatively move the signal.Notify setup to after the subscribe call if you prefer that flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/subscribe.go`:
- Around line 159-160: The signal handling is registered before the blocking
HTTP subscribe POST and the sigChan isn't consumed until after the request, so
Ctrl+C won't abort the request; fix by creating the HTTP request with a
cancelable context and wiring signals to cancel it (use context.WithCancel or
signal.NotifyContext) so the in-flight subscribe POST can be aborted on
SIGINT/SIGTERM; specifically update the code around sigChan/signal.Notify and
the subscribe HTTP request (use http.NewRequestWithContext or pass the context
into the function that performs the subscribe call) so the request listens to
the cancel context rather than relying on a later-read sigChan, or alternatively
move the signal.Notify setup to after the subscribe call if you prefer that
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebb8ce8b-6fdc-4bb0-9564-617a869d6580
📒 Files selected for processing (4)
cmd/list.gocmd/publish.gocmd/subscribe.gocmd/utils.go
1 participant
Summary
http.DefaultClient(zero timeout) with a sharedhttpClientconfigured with a 30-second timeout inpublish,subscribe, andlist-topicscommands. This prevents the CLI from hanging indefinitely when the remote server is unreachable or slow to respond.bytes.NewReaderinstead ofstrings.NewReader(string(...)), avoiding a redundant heap copy of the request payload.publish.go— the onlyreturn errwithout context wrapping in the entire function — to provide a descriptive message consistent with all other error returns.io.ReadAllerrors inpublishandsubscribeinstead of silently discarding them with_ =, which previously produced empty error messages when the response body couldn't be read.Motivation
Several commands (
health,tracer,update) already use properly configured HTTP clients with explicit timeouts. Thepublish,subscribe, andlist-topicscommands were the only ones still usinghttp.DefaultClient, which has no timeout on any stage (DNS, TLS, response headers, body). This is an internal inconsistency and a real operational issue — a user runningmump2p publishagainst a misconfigured or unreachable server would have their terminal freeze with no recovery.The error handling fixes are in the same code paths and follow patterns already established elsewhere in the codebase (
list.go:64,health.go:54).Changes
cmd/utils.gohttpClientwith 30s timeoutcmd/publish.goDefaultClient, usebytes.NewReader, wrap error, checkio.ReadAllcmd/subscribe.goDefaultClient(4 sites), checkio.ReadAllcmd/list.goDefaultClient(2 sites)Test plan
go build ./...— compiles without errorsgo vet ./...— no static analysis warningsgo test ./...— all unit tests passmump2p publish --topic=test --message=helloagainst unreachable server now times out after 30s instead of hangingSummary by CodeRabbit
Bug Fixes
Chores