rpcclient: add context.Context support for HTTP POST mode#2506
rpcclient: add context.Context support for HTTP POST mode#2506seeforschauer wants to merge 6 commits into
Conversation
Add SendCmdWithContext and GetBlockCountWithContext methods that propagate a context.Context to the underlying HTTP request. This allows callers to cancel or timeout individual RPC calls. Changes: - Add ctx field to jsonRequest struct - Use http.NewRequestWithContext in handleSendPostMessage - Respect context cancellation during retry backoff - Add SendCmdWithContext to Client - Add GetBlockCountWithContext as the first context-aware method - Existing methods are unchanged (non-breaking) Fixes btcsuite#2499
Refactor SendCmd to delegate to SendCmdWithContext instead of duplicating the marshalling logic. SendCmdWithContext is now the single implementation; SendCmd passes context.Background(). Add a context error check after httpClient.Do in the retry loop so a cancelled context bails out immediately instead of retrying. Use camelCase for test function names.
|
Diff looks good, tests passing. Nice work @seeforschauer! |
|
@Roasbeef all four items addressed — mind taking another look when you get a chance? |
kcalvinalvin
left a comment
There was a problem hiding this comment.
Looks good overall. Some minor test related issues that I saw
|
@kcalvinalvin pls review |
…-support # Conflicts: # rpcclient/infrastructure.go
|
@Roasbeef would you please review once more? |
| ## Examples | ||
|
|
||
| * [Decode Example](http://godoc.org/github.com/btcsuite/btcd/address/v2/base58#example-Decode) | ||
| * [Decode Example](http://godoc.org/github.com/btcsuite/btcd/address/v2/base58#example-Decode) |
There was a problem hiding this comment.
I get that these are easy (probably automatic) fixes but it does make reviewing a bit more annoying since I believe all the reviewers read git diffs commit by commit
|
The code looks good. Though the tests aren't doing much. These test all pass even when the context is dropped in GetBlockCountWithConext:
Ran the tests with this code: Like the tests are implementing the handler in the test and is just checking if that works. I don't think that's useful really. I feel that the actual code is mergeable. The tests... eh. Another nit is the git commit format. I'd prefer the follow up commits to be amended into the originals. But these aren't a blocker. |
|
updated |
|
Hi @seeforschauer why is this pr in draft status? |
|
The latest commit ccbcd30 fixes the test problems I pointed out. LGTM. Still not a fan of the commit order but that's not a stopper |
Change Description
Add
context.Contextsupport to rpcclient's HTTP POST mode, allowing callers to cancel or timeout individual RPC calls. Fixes #2499.Currently, rpcclient methods like
GetBlockCount()don't accept a context, which means callers have no way to cancel or set deadlines on individual RPC calls. In HTTP POST mode,handleSendPostMessageuseshttp.NewRequestwithout context, and the 10-retry loop doesn't respect external cancellation.Changes
ctxfield tojsonRequeststructhttp.NewRequestWithContextinhandleSendPostMessagewhen ctx is setselectonreqCtx.Done())SendCmdWithContext(ctx, cmd)toClientGetBlockCountWithContext(ctx)as the first context-aware method variantDesign decisions
SendCmdWithContextmirrorsSendCmdwith an added ctx parameterctxis nil (e.g., from existingSendCmdcallers),context.Background()is used as fallback*WithContextmethods for other RPC calls can be added incrementally in follow-up PRsSteps to Test
Pull Request Checklist
Testing
Code Style and Documentation