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 all 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
14 changes: 5 additions & 9 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,24 +47,18 @@ 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
resp, err := http.DefaultClient.Do(request)
if err != nil {
return nil, fmt.Errorf("failed to issue request: %w", err)
}
defer rpc.CleanlyCloseBody(resp.Body)

// 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()
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()
return nil, err
}
return metrics, resp.Body.Close()
return parser.TextToMetricFamilies(resp.Body)
}
4 changes: 3 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 @@ -110,11 +111,12 @@ func queryLoki(
req.Header.Set("Authorization", "Basic "+auth)

// Execute request
//nolint:bodyclose // body is closed via rpc.CleanlyCloseBody
resp, err := http.DefaultClient.Do(req)
if err != nil {
return 0, stacktrace.Errorf("failed to execute request: %w", err)
}
defer resp.Body.Close()
defer rpc.CleanlyCloseBody(resp.Body)

// Read and parse response
body, err := io.ReadAll(resp.Body)
Expand Down
4 changes: 3 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 @@ -587,11 +588,12 @@ func checkReadiness(ctx context.Context, url string) (bool, string, error) {
return false, "", stacktrace.Wrap(err)
}

//nolint:bodyclose // body is closed via rpc.CleanlyCloseBody
resp, err := http.DefaultClient.Do(req)
if err != nil {
return false, "", stacktrace.Errorf("request failed: %w", err)
}
defer resp.Body.Close()
defer rpc.CleanlyCloseBody(resp.Body)

body, err := io.ReadAll(resp.Body)
if err != nil {
Expand Down
5 changes: 4 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 @@ -220,11 +221,13 @@ func (n *Node) SaveMetricsSnapshot(ctx context.Context) error {
if err != nil {
return stacktrace.Wrap(err)
}
//nolint:bodyclose // body is closed via rpc.CleanlyCloseBody
resp, err := http.DefaultClient.Do(req)
if err != nil {
return stacktrace.Wrap(err)
}
defer resp.Body.Close()
defer rpc.CleanlyCloseBody(resp.Body)

body, err := io.ReadAll(resp.Body)
if err != nil {
return stacktrace.Wrap(err)
Expand Down
4 changes: 3 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 @@ -27,11 +28,12 @@ func (r *ifConfigResolver) Resolve(ctx context.Context) (netip.Addr, error) {
return netip.Addr{}, err
}

//nolint:bodyclose // body is closed via rpc.CleanlyCloseBody
resp, err := http.DefaultClient.Do(req)
if err != nil {
return netip.Addr{}, err
}
defer resp.Body.Close()
defer 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
18 changes: 13 additions & 5 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) {
_, _ = io.Copy(io.Discard, body)
_ = body.Close()
}

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

//nolint:bodyclose // body is closed via CleanlyCloseBody
resp, err := http.DefaultClient.Do(request)
if err != nil {
return fmt.Errorf("failed to issue request: %w", err)
}
defer CleanlyCloseBody(resp.Body)

// 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()
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()
return fmt.Errorf("failed to decode client response: %w", err)
}
return resp.Body.Close()

return nil
}