rpcclient: make shutdown interrupt in-flight POSTs by wydengyre · Pull Request #2451 · btcsuite/btcd

wydengyre · 2025-10-30T15:14:14Z

Change Description

Modify the rpcclient http POST call to ensure that a shutdown immediately interrupts in-flight requests, which otherwise would have to wait until timeout.

This PR includes the change in #2450 as it is necessary to ensure interruption during Dial.

Steps to Test

The test suite has an added test to ensure this works.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.

📝 Please see our Contribution Guidelines for further guidance.

coveralls · 2025-10-30T15:18:45Z

Pull Request Test Coverage Report for Build 21215011456

Details

21 of 24 (87.5%) changed or added relevant lines in 1 file are covered.
85 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 54.972%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rpcclient/infrastructure.go	21	24	87.5%

Files with Coverage Reduction	New Missed Lines	%
btcutil/gcs/gcs.go	1	80.95%
rpcclient/infrastructure.go	84	48.49%

Totals
Change from base Build 20942501138:	0.02%
Covered Lines:	31217
Relevant Lines:	56787

💛 - Coveralls

jcvernaleo

Looks good.

OK

kcalvinalvin · 2026-01-10T05:06:54Z

rpcclient/infrastructure.go

+		httpReq, err = http.NewRequestWithContext(ctx, "POST", httpURL, bodyReader)
 		if err != nil {
+			// We must observe the contract that shutdown returns ErrClientShutdown.
+			if errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), ErrClientShutdown) {


This check is unneeded since http.NewRequestWithContext will never return a error that's of type context.Canceled.

Removed in f87d914

kcalvinalvin · 2026-01-10T05:09:26Z

Checked:

1: The added TestHTTPPostShutdownInterruptsPendingRequest test is correct.
2: Sanity checked the changes in infrastructure.go.

One nitpicky gripe is maybe the test could be commented better.

Looks good overall except for that one unneeded check.

Drop unreachable context-canceled mapping after request creation. Clarify the HTTP POST shutdown test flow with brief comments.

wydengyre · 2026-01-21T15:19:26Z

Checked:

1: The added TestHTTPPostShutdownInterruptsPendingRequest test is correct. 2: Sanity checked the changes in infrastructure.go.

One nitpicky gripe is maybe the test could be commented better.

Looks good overall except for that one unneeded check.

@kcalvinalvin thanks for your review.

I've addressed your comments in f87d914

Can you re-review?

seeforschauer · 2026-03-19T08:20:43Z

Hey @wydengyre, nice work on this — we hit the same pain point in production.

We run a HAProxy health checker that polls BTC nodes via GetBlockCount(). Each probe spawns a goroutine; when a node hangs, GetBlockCount blocks for ~600s (retry loop in handleSendPostMessage — 10 attempts x 60s HTTP timeout) while new probes keep arriving. We observed 117 leaked goroutines on staging.

We searched the issue tracker and PRs for existing context support work — found #2450 (dial timeout, merged), this PR (shutdown interrupt), and #1323 (connection reuse), but nothing addressing per-request context.Context for RPC methods. The gap is clear: callers currently have no way to cancel or timeout an individual GetBlockCount/SendRawTransaction/etc. call.

Your PR fixes shutdown propagation, which is great. The transport plumbing is almost there with http.NewRequestWithContext. Would you or maintainers be open to extending this to per-request context support? Something like:

func (c *Client) GetBlockCountWithContext(ctx context.Context) (int64, error)

Minimal changes: ctx field on jsonRequest, ctx.Done() check in the retry loop, SendCmdCtx variant. Non-breaking — existing methods keep working. ~80-100 lines.

Happy to open an issue + PR for this if there's interest. We worked around it with a singleflight pattern at our layer, but the proper fix belongs here.

wydengyre · 2026-03-19T12:48:57Z

Would you or maintainers be open to extending this to per-request context support?

@seeforschauer I had the same problem as you, and actually have a private library that fixes it in a backward-compatible way. But I'm not affiliated with this project in any official way, so sadly the decision to support such a feature is outside my influence. I would certainly vote in favor of it, though, as it's both pragmatic/necessary and more "Go-like".

seeforschauer · 2026-03-20T07:49:36Z

Thanks for the support, and good to know you've solved it independently — validates the approach.

I went ahead and opened #2499 to formalize the request for per-request context.Context support. If you'd be open to sharing your backward-compatible approach (or collaborating on a PR), happy to coordinate there.

In the meantime, pinging @kcalvinalvin — any thoughts on merging this PR as-is and tracking context support separately in #2499?

starius

The code looks good! I found a couple of remaining cases of returning non-remapped errors and also gaps in test coverage.

Can you consolidate errors remapping in one place, so there are no leaks of non-remapped error from handleSendPostMessage (in case we add more return operators in the middle of it later).

Could you also rebase the PR so it is easier to review, please?

starius · 2026-03-20T20:30:29Z

rpcclient/infrastructure.go

 		httpResponse, err = c.httpClient.Do(httpReq)

 		// Quit the retry loop on success or if we can't retry anymore.
 		if err == nil || i == tries-1 {


If errors.Is(err, context.Canceled) && errors.Is(context.Cause(ctx), ErrClientShutdown) && i == tries-1, the added error remapping logic is skipped. The code returns context.Canceled in this case, but it is expected to return ErrClientShutdown instead.

starius · 2026-03-20T20:31:50Z

rpcclient/infrastructure.go

 	}

 	// Read the raw bytes and close the response.
 	respBytes, err := io.ReadAll(httpResponse.Body)


If ctx is cancelled in the middle of io.ReadAll running, we return fmt.Errorf("error reading json reply: ..., but I think we should also return ErrClientShutdown in this case, right?

starius · 2026-03-20T20:34:13Z

rpcclient/infrastructure_test.go

+	result, err := future.Receive()
+	require.Zero(t, result)
+	require.ErrorContains(t, err, ErrClientShutdown.Error())
+}


I propose to add more test coverage of the edge cases:

Cancellation while waiting in retry backoff path (ctx.Done() select branch).

Cancellation on final retry attempt (i == tries-1).

Cancellation during response body read after a successful Do.

starius · 2026-03-24T20:58:47Z

I'm going to copy the commits from this PR on top of #2500 and resolve the remaining comments.

@wydengyre If you already addressed some of them, please push, so I have the latest version of your commits.

wydengyre · 2026-03-24T21:11:37Z

@wydengyre If you already addressed some of them, please push, so I have the latest version of your commits.

I have not, sorry. Please go ahead.

starius · 2026-03-25T04:52:44Z

This PR was integrated into #2500

rpcclient: support canceling in-flight http requests

b2c2b50

Merge branch 'master' into btc-client-respect-context

460ef31

jcvernaleo approved these changes Nov 12, 2025

View reviewed changes

Roasbeef requested a review from kcalvinalvin December 18, 2025 02:48

kcalvinalvin reviewed Jan 10, 2026

View reviewed changes

wydengyre added 2 commits January 21, 2026 12:13

Merge branch 'master' into btc-client-respect-context

59ca43b

rpcclient: refine http post shutdown test

f87d914

Drop unreachable context-canceled mapping after request creation. Clarify the HTTP POST shutdown test flow with brief comments.

wydengyre requested review from jcvernaleo and kcalvinalvin January 21, 2026 15:19

seeforschauer mentioned this pull request Mar 19, 2026

rpcclient: add context.Context support to RPC methods #2499

Open

saubyk assigned wydengyre Mar 20, 2026

saubyk added this to the v0.25.1 milestone Mar 20, 2026

starius reviewed Mar 20, 2026

View reviewed changes

starius mentioned this pull request Mar 20, 2026

rpcclient: fix several synchronization bugs #2500

Open

7 tasks

Roasbeef closed this Mar 25, 2026

Conversation

wydengyre commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

coveralls commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21215011456

Details

💛 - Coveralls

Uh oh!

jcvernaleo left a comment

Choose a reason for hiding this comment

Uh oh!

kcalvinalvin Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

wydengyre Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

kcalvinalvin commented Jan 10, 2026

Uh oh!

wydengyre commented Jan 21, 2026

Uh oh!

seeforschauer commented Mar 19, 2026

Uh oh!

wydengyre commented Mar 19, 2026

Uh oh!

seeforschauer commented Mar 20, 2026

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

starius Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

starius Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

starius Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

starius commented Mar 24, 2026

Uh oh!

wydengyre commented Mar 24, 2026

Uh oh!

starius commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wydengyre commented Oct 30, 2025 •

edited

Loading

coveralls commented Oct 30, 2025 •

edited

Loading