Skip to content
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.

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

fix: node gateway error handling improvements #124

Merged
merged 6 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/golangci-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ jobs:
- name: golangci-lint
uses: golangci/golangci-lint-action@v3
with:
version: v1.48.0
version: v1.55.2
args: ./...
4 changes: 1 addition & 3 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ linters:
disable-all: true
enable:
- bodyclose
- deadcode
- depguard
# - depguard
- dogsled
- dupl
- errcheck
Expand Down Expand Up @@ -55,7 +54,6 @@ linters:
- typecheck
- unconvert
- unparam
- varcheck
- whitespace
- wsl

Expand Down
50 changes: 13 additions & 37 deletions internal/jsonrpc/jsonrpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ package jsonrpc
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
)

const JSONRPCVersion = "2.0"
Expand Down Expand Up @@ -102,73 +101,50 @@ type DecodeError struct {
Content []byte // Content that couldn't be decoded.
}

func NewDecodeError(err error, content []byte) DecodeError {
return DecodeError{
func NewDecodeError(err error, content []byte) *DecodeError {
return &DecodeError{
Err: err,
Content: content,
}
}

func (e DecodeError) Error() string {
func (e *DecodeError) Error() string {
return fmt.Sprintf("decode error: %s, content: %s", e.Err.Error(), string(e.Content))
}

func DecodeRequestBody(req *http.Request) (RequestBody, error) {
// No need to close the request body, the Server implementation will take care of it.
requestRawBytes, err := io.ReadAll(req.Body)

if err != nil {
return nil, NewDecodeError(err, requestRawBytes)
}

var body *SingleRequestBody

func DecodeRequestBody(requestBodyRawBytes []byte) (RequestBody, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moving the io.ReadAll call out of this function because errors reading data out of the stream were being conflated with real decoding errors.

Example from logs: "error":"decode error: http2: server sent GOAWAY and closed the connection; LastStreamID=19999, ErrCode=NO_ERROR, debug=\"\",

This is an error from io.ReadAll. It's an error on the connection level, not with the contents of the response.

// Try non-batch first as these are probably more common.
if body, err = decode[SingleRequestBody](requestRawBytes); err == nil {
if body, err := decode[SingleRequestBody](requestBodyRawBytes); err == nil {
return body, nil
}

var batchBody *[]SingleRequestBody

if batchBody, err = decode[[]SingleRequestBody](requestRawBytes); err == nil {
if batchBody, err := decode[[]SingleRequestBody](requestBodyRawBytes); err == nil {
return &BatchRequestBody{
Requests: *batchBody,
}, nil
}

return nil, NewDecodeError(err, requestRawBytes)
return nil, NewDecodeError(errors.New("unexpected decoding request error"), requestBodyRawBytes)
}

func DecodeResponseBody(resp *http.Response) (ResponseBody, error) {
// As per the spec, it is the caller's responsibility to close the response body.
defer resp.Body.Close()
responseRawBytes, err := io.ReadAll(resp.Body)

if err != nil {
return nil, NewDecodeError(err, responseRawBytes)
}

func DecodeResponseBody(responseBodyRawBytes []byte) (ResponseBody, error) {
// Empty JSON RPC responses are valid for "Notifications" (requests without "ID") https://www.jsonrpc.org/specification#notification
if len(responseRawBytes) == 0 {
if len(responseBodyRawBytes) == 0 {
return nil, nil
}

var body *SingleResponseBody

// Try non-batch first as these are probably more common.
if body, err = decode[SingleResponseBody](responseRawBytes); err == nil {
if body, err := decode[SingleResponseBody](responseBodyRawBytes); err == nil {
return body, nil
}

var batchBody *[]SingleResponseBody

if batchBody, err = decode[[]SingleResponseBody](responseRawBytes); err == nil {
if batchBody, err := decode[[]SingleResponseBody](responseBodyRawBytes); err == nil {
return &BatchResponseBody{
Responses: *batchBody,
}, nil
}

return nil, NewDecodeError(err, responseRawBytes)
return nil, NewDecodeError(errors.New("unexpected decoding response error"), responseBodyRawBytes)
}

func decode[T Decodable](rawBytes []byte) (*T, error) {
Expand Down
27 changes: 12 additions & 15 deletions internal/jsonrpc/jsonrpc_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
package jsonrpc

import (
"bytes"
"encoding/json"
"io"
"net/http"
"testing"

"github.com/samber/lo"
Expand Down Expand Up @@ -91,10 +88,7 @@ func TestEncodeAndDecodeRequests(t *testing.T) {
},
},
} {
req := http.Request{
Body: io.NopCloser(bytes.NewReader([]byte(tc.body))),
}
decoded, err := DecodeRequestBody(&req)
decoded, err := DecodeRequestBody([]byte(tc.body))

assert.Nil(t, err)
assert.Equal(t, tc.expectedRequest, decoded)
Expand Down Expand Up @@ -143,6 +137,11 @@ func TestEncodeAndDecodeResponses(t *testing.T) {
},
},
},
{
testName: "empty response in batch",
body: ``,
expectedResponse: nil,
},
{
testName: "batch responses",
body: "[" +
Expand Down Expand Up @@ -171,18 +170,16 @@ func TestEncodeAndDecodeResponses(t *testing.T) {
},
},
} {
resp := http.Response{
Body: io.NopCloser(bytes.NewReader([]byte(tc.body))),
}

decoded, err := DecodeResponseBody(&resp)
decoded, err := DecodeResponseBody([]byte(tc.body))

assert.Nil(t, err)
assert.Equal(t, tc.expectedResponse, decoded)

encoded, err := decoded.Encode()
if decoded != nil {
encoded, err := decoded.Encode()

assert.Nil(t, err)
assert.Equal(t, tc.body, string(encoded))
assert.Nil(t, err)
assert.Equal(t, tc.body, string(encoded))
}
}
}
2 changes: 1 addition & 1 deletion internal/metadata/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (c *ChainMetadataStore) ProcessBlockHeightUpdate(groupID, upstreamID string
}
}

func (c *ChainMetadataStore) ProcessErrorUpdate(groupID, upstreamID string, err error) {
func (c *ChainMetadataStore) ProcessErrorUpdate(_, upstreamID string, err error) {
c.opChannel <- func() {
c.updateErrorForUpstream(upstreamID, err)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/metrics/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ var (
"Batches are deconstructed to single JSON RPC requests for this metric.",
},
// jsonrpc_method is "batch" for batch requests
[]string{"chain_name", "client", "upstream_id", "url", "jsonrpc_method", "response_code", "jsonrpc_error_code"},
[]string{"chain_name", "client", "upstream_id", "url", "jsonrpc_method", "jsonrpc_error_code"},
Copy link
Member Author

@brianluong brianluong Dec 19, 2023

Choose a reason for hiding this comment

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

Didn't feel HTTP code on a JSON RPC metric was necessary.

Copy link
Member

Choose a reason for hiding this comment

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

agreed!

)

upstreamRPCDuration = promauto.NewHistogramVec(
Expand Down
23 changes: 17 additions & 6 deletions internal/mocks/BlockHeightChecker.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 9 additions & 6 deletions internal/mocks/Checker.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 21 additions & 6 deletions internal/mocks/EthClient.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 9 additions & 6 deletions internal/mocks/HTTPClient.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading