Skip to content

Avoid unnecessary RST_STREAM and PING frames causing GOAWAY messages #4480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 10, 2025
Merged

Avoid unnecessary RST_STREAM and PING frames causing GOAWAY messages #4480

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dad46a0
Ensure full response body is always read to avoid unnecessary RST_STR…
michaelkaplan13 Nov 6, 2025
3b7c182
Close helper
michaelkaplan13 Nov 7, 2025
5e980d7
nit
michaelkaplan13 Nov 7, 2025
b3e5936
Always ignore body.Close() errors
michaelkaplan13 Nov 7, 2025
ead4b62
simplify return
michaelkaplan13 Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions api/metrics/client.go
View file
Open in desktop
Copy link
Contributor

@joshua-kim joshua-kim Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this client behavior update in this file? This is only used in e2e tests so I feel like this is over-kill for an environment that is just a smoke test.

Copy link
Contributor Author

@michaelkaplan13 michaelkaplan13 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes a difference either way practically speaking, but I think applying best practices everywhere is better than not, particularly now that we're just uniformly deferring the call to the helper everywhere (which I realize is a change from when this comment was left). Thoughts?

Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (

"github.com/prometheus/common/expfmt"

"github.com/ava-labs/avalanchego/utils/rpc"

dto "github.com/prometheus/client_model/go"
)

Expand Down Expand Up @@ -45,6 +47,7 @@ func (c *Client) GetMetrics(ctx context.Context) (map[string]*dto.MetricFamily,
return nil, fmt.Errorf("failed to create request: %w", err)
}

//nolint:bodyclose // body is closed via rpc.CleanlyCloseBody in all code paths
resp, err := http.DefaultClient.Do(request)
if err != nil {
return nil, fmt.Errorf("failed to issue request: %w", err)
Expand All @@ -53,16 +56,17 @@ func (c *Client) GetMetrics(ctx context.Context) (map[string]*dto.MetricFamily,
// Return an error for any non successful status code
if resp.StatusCode < 200 || resp.StatusCode > 299 {
// Drop any error during close to report the original error
_ = resp.Body.Close()
_ = rpc.CleanlyCloseBody(resp.Body)
return nil, fmt.Errorf("received status code: %d", resp.StatusCode)
}

var parser expfmt.TextParser
metrics, err := parser.TextToMetricFamilies(resp.Body)
if err != nil {
// Drop any error during close to report the original error
_ = resp.Body.Close()
_ = rpc.CleanlyCloseBody(resp.Body)
return nil, err
}
return metrics, resp.Body.Close()

return metrics, rpc.CleanlyCloseBody(resp.Body)
}
3 changes: 2 additions & 1 deletion tests/fixture/tmpnet/check_monitoring.go
View file
Open in desktop
Copy link
Contributor

@joshua-kim joshua-kim Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment in this diff in this package w.r.t io.ReadAll

Copy link
Contributor Author

@michaelkaplan13 michaelkaplan13 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, now that we have the CleanlyCloseBody helper, I think it's most defensive to use it uniformly across the board rather than depending on the fact that the body is always fully read otherwise.

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/ava-labs/avalanchego/tests/fixture/stacktrace"
"github.com/ava-labs/avalanchego/utils/logging"
"github.com/ava-labs/avalanchego/utils/rpc"
)

type getCountFunc func() (int, error)
Expand Down Expand Up @@ -114,7 +115,7 @@ func queryLoki(
if err != nil {
return 0, stacktrace.Errorf("failed to execute request: %w", err)
}
defer resp.Body.Close()
defer func() { _ = rpc.CleanlyCloseBody(resp.Body) }()

// Read and parse response
body, err := io.ReadAll(resp.Body)
Expand Down
3 changes: 2 additions & 1 deletion tests/fixture/tmpnet/monitor_processes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/ava-labs/avalanchego/tests/fixture/stacktrace"
"github.com/ava-labs/avalanchego/utils/logging"
"github.com/ava-labs/avalanchego/utils/perms"
"github.com/ava-labs/avalanchego/utils/rpc"
)

const (
Expand Down Expand Up @@ -591,7 +592,7 @@ func checkReadiness(ctx context.Context, url string) (bool, string, error) {
if err != nil {
return false, "", stacktrace.Errorf("request failed: %w", err)
}
defer resp.Body.Close()
defer func() { _ = rpc.CleanlyCloseBody(resp.Body) }()

body, err := io.ReadAll(resp.Body)
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion tests/fixture/tmpnet/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/ava-labs/avalanchego/tests/fixture/stacktrace"
"github.com/ava-labs/avalanchego/utils/constants"
"github.com/ava-labs/avalanchego/utils/crypto/bls/signer/localsigner"
"github.com/ava-labs/avalanchego/utils/rpc"
"github.com/ava-labs/avalanchego/vms/platformvm/signer"
)

Expand Down Expand Up @@ -224,7 +225,8 @@ func (n *Node) SaveMetricsSnapshot(ctx context.Context) error {
if err != nil {
return stacktrace.Wrap(err)
}
defer resp.Body.Close()
defer func() { _ = rpc.CleanlyCloseBody(resp.Body) }()

body, err := io.ReadAll(resp.Body)
if err != nil {
return stacktrace.Wrap(err)
Expand Down
3 changes: 2 additions & 1 deletion utils/dynamicip/ifconfig_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"strings"

"github.com/ava-labs/avalanchego/utils/ips"
"github.com/ava-labs/avalanchego/utils/rpc"
)

var _ Resolver = (*ifConfigResolver)(nil)
Expand All @@ -31,7 +32,7 @@ func (r *ifConfigResolver) Resolve(ctx context.Context) (netip.Addr, error) {
if err != nil {
return netip.Addr{}, err
}
defer resp.Body.Close()
defer func() { _ = rpc.CleanlyCloseBody(resp.Body) }()

ipBytes, err := io.ReadAll(resp.Body)
Copy link
Contributor

@joshua-kim joshua-kim Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already read until EOF here - do we still need to drain the response body?

Copy link
Contributor Author

@michaelkaplan13 michaelkaplan13 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have the CleanlyCloseBody helper, I think it's most defensive to use it uniformly across the board rather than depending on the fact that the body is always fully read otherwise.

if err != nil {
Expand Down
17 changes: 14 additions & 3 deletions utils/rpc/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,21 @@ import (
"bytes"
"context"
"fmt"
"io"
"net/http"
"net/url"

rpc "github.com/gorilla/rpc/v2/json2"
)

// CleanlyCloseBody avoids sending unnecessary RST_STREAM and PING frames by ensuring
// the whole body is read before being closed.
// See https://blog.cloudflare.com/go-and-enhance-your-calm/#reading-bodies-in-go-can-be-unintuitive
func CleanlyCloseBody(body io.ReadCloser) error {
_, _ = io.Copy(io.Discard, body)
return body.Close()
}
Copy link
Contributor

@StephenButtolph StephenButtolph Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember when first adding the error returning logic to the prod clients that it was kind of "we should probably return the error if one happens"... But honestly, if the client was able to get the response... I feel like dropping the error isn't unreasonable.

Should we just ignore this everywhere and make this function not return an error? We could ignore the error once here, and then everywhere just defer CleanlyCloseBody(resp.Body)

Suggested change
// CleanlyCloseBody avoids sending unnecessary RST_STREAM and PING frames by ensuring
// the whole body is read before being closed.
// See https://blog.cloudflare.com/go-and-enhance-your-calm/#reading-bodies-in-go-can-be-unintuitive
func CleanlyCloseBody(body io.ReadCloser) error {
_, _ = io.Copy(io.Discard, body)
return body.Close()
}
// CleanlyCloseBody avoids sending unnecessary RST_STREAM and PING frames by
// ensuring the whole body is read before being closed.
// See https://blog.cloudflare.com/go-and-enhance-your-calm/#reading-bodies-in-go-can-be-unintuitive
func CleanlyCloseBody(body io.ReadCloser) {
_, _ = io.Copy(io.Discard, body)
_ = body.Close()
}

Copy link
Contributor Author

@michaelkaplan13 michaelkaplan13 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very reasonable and I'm happy to make this change because it cleans up the code, though I do think it's a potential behavior change outside the scope of the intention of this PR.

Copy link
Contributor

@joshua-kim joshua-kim Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to my testing, but io.Copy can return a cancelation error since it respects the context - so we might want to bubble up that error instead.


func SendJSONRequest(
ctx context.Context,
uri *url.URL,
Expand Down Expand Up @@ -42,6 +51,7 @@ func SendJSONRequest(
request.Header = ops.headers
request.Header.Set("Content-Type", "application/json")

//nolint:bodyclose // body is closed via CleanlyCloseBody in all code paths
resp, err := http.DefaultClient.Do(request)
if err != nil {
return fmt.Errorf("failed to issue request: %w", err)
Expand All @@ -50,14 +60,15 @@ func SendJSONRequest(
// Return an error for any non successful status code
if resp.StatusCode < 200 || resp.StatusCode > 299 {
// Drop any error during close to report the original error
_ = resp.Body.Close()
_ = CleanlyCloseBody(resp.Body)
return fmt.Errorf("received status code: %d", resp.StatusCode)
}

if err := rpc.DecodeClientResponse(resp.Body, reply); err != nil {
// Drop any error during close to report the original error
_ = resp.Body.Close()
_ = CleanlyCloseBody(resp.Body)
return fmt.Errorf("failed to decode client response: %w", err)
}
return resp.Body.Close()

return CleanlyCloseBody(resp.Body)
}