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

Consider returning http errors for certain rpc errors #11792

Closed
bowenwang1996 opened this issue Jul 16, 2024 · 3 comments · Fixed by #11806 or #11822
Closed

Consider returning http errors for certain rpc errors #11792

bowenwang1996 opened this issue Jul 16, 2024 · 3 comments · Fixed by #11806 or #11822

Comments

@bowenwang1996
Copy link
Collaborator

Currently when a rpc request is processed, we never return a http error. However, this makes it confusing to rpc operators, who may see very low "error rate" in their monitoring when there may actually be a nontrivial amount of requests that return a timeout error with a 200 response code. We may want to consider returning a HttpError in that function for certain rpc errors like timeout. Of course there are legitimate errors such as "account not found" that make sense to be included in a response body with a 200.

github-merge-queue bot pushed a commit that referenced this issue Jul 20, 2024
…11806)

This PR updates the RPC handler to return appropriate HTTP errors for
specific RPC issues, improving error visibility for operators

closes #11792

---------

Co-authored-by: Shreyan Gupta <[email protected]>
@frol
Copy link
Collaborator

frol commented Jul 20, 2024

Technically speaking, JSON RPC doesn't use HTTP status codes to indicate the result (at the very least, it is due to the fact that JSON RPC supports requests batching): https://www.jsonrpc.org/specification

@bowenwang1996 by timeout response do you refer to tx-status, broadcast-tx timeouts? They are also part of regular operations flow, aren't they?

I reopened this issue due to #11806 (comment)

@frol
Copy link
Collaborator

frol commented Jul 21, 2024

@bowenwang1996 FYI, changing the HTTP status codes will be a semi-breaking change. For example, non-ok response will be now handled here in near-api-js instead of further down the line. This means that all those legacy projects will do more retries on Timeout - one exponentionBackoff from fetchJson and another one from sendJsonRpc (it is is calling fetchJson)

@bowenwang1996
Copy link
Collaborator Author

@bowenwang1996 by timeout response do you refer to tx-status, broadcast-tx timeouts? They are also part of regular operations flow, aren't they?

I think it mostly indicates an error. This would happen either because the rpc node is falling behind, or because the network is congested. In either case it seems that an error makes sense. Do you have suggestions regarding how to distinguish between errors?

github-merge-queue bot pushed a commit that referenced this issue Jul 25, 2024
This PR fixes a small issue that was introduced in #11806

Internal error - 500 status code
Handler error - 408 if it's a `TIMEOUT_ERROR`, otherwise 200
Request validation error - 400 status code

closes #11792
VanBarbascu pushed a commit that referenced this issue Jul 31, 2024
This PR fixes a small issue that was introduced in #11806

Internal error - 500 status code
Handler error - 408 if it's a `TIMEOUT_ERROR`, otherwise 200
Request validation error - 400 status code

closes #11792
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants