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

RPC 'tooBusy' response should have 50x (503?) status code #4005

Closed
WietseWind opened this issue Nov 28, 2021 · 5 comments · Fixed by #4143
Closed

RPC 'tooBusy' response should have 50x (503?) status code #4005

WietseWind opened this issue Nov 28, 2021 · 5 comments · Fixed by #4143

Comments

@WietseWind
Copy link
Member

WietseWind commented Nov 28, 2021

Currently, if rippled (over RPC) returns a tooBusy response, like below, it does so with a HTTP 200 status code. This doesn't deserve a 200 status code, all is not OK, and one is definitely not getting the expected response.

I propose returning a HTTP 503 status code along with the tooBusy error message. Any error for that matter should have a non-200 HTTP status code.

By not assigning a non-200 status code:

  • Clients are mislead on the validity of the response
  • Reverse proxies / load balancers can't auto failover to another node as they get a 200 response
{
  "result": {
    "error": "tooBusy",
    "error_code": 9,
    "error_message": "The server is too busy to help you now.",
    "request": {
      "account": "rTooLkitCksh5mQa67eaa2JaWHDBnHkpy",
      "command": "account_tx",
      "ledger_index_max": 61974273,
      "limit": 20
    },
    "status": "error"
  }
}
@cjcobb23
Copy link
Contributor

I agree that tooBusy should have a 50x status code. I don't know that every error should though. I don't think an account not found or transaction not found error means anything is actually wrong with the server, without knowing whether that data should have been found.

@intelliot
Copy link
Collaborator

intelliot commented Nov 30, 2021

If we want to follow the HTTP status codes:

  • 200 OK means the request was successfully received, understood, and accepted. The response includes the requested resource
  • 503 Service Unavailable is correct for a server that is too busy
  • 404 Not Found may be used for account not found or transaction not found
  • 204 No Content may be used for e.g. account_tx with no txs in the requested range of ledgers

@WietseWind
Copy link
Member Author

WietseWind commented Nov 30, 2021

I agree that tooBusy should have a 50x status code. I don't know that every error should though. I don't think an account not found or transaction not found error means anything is actually wrong with the server, without knowing whether that data should have been found.

Just had the same thought. There's a distinction between "unexpected", infra-like error (must have an error code) and 'logic' error (200, but with an error).

@intelliot I think the 404 for e.g. account not found is a bad idea as that would trigger auto-retry/failover load balancers to retry with another uplink.

@intelliot @cjcobb23 I think this could be a good thing to keep in mind deciding if something needs a non-200 status code: "would the same call on another node potentially yield different (better, expected) results?"? If so: non-200 (error, so next uplink can be queried). If not (e.g. account doesn't exist, address encoding wrong, ledger sequence in the future, name it...): 200 status code but potentially with an error explaining what was wrong.

So good candidates for a non-200 status code are, for example:

  • server tooBusy
  • server not synced to the network
  • history (ledgers) missing for the queried range
  • ...

@syeduali93
Copy link

Is this related to the recent scalability issues the XRPL has been having with Trustlines?

@WietseWind
Copy link
Member Author

Is this related to the recent scalability issues the XRPL has been having with Trustlines?

No.

scottschurr added a commit to scottschurr/rippled that referenced this issue Apr 5, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error codes to associate
themselves with a non-200 HTTP status code.  There are a few
RPC responses in addition to tooBusy that now have non-200
HTTP status codes.

This is a work in progress.  I need to make sure this works with
rippled RPC version 2.  I've not yet done that testing.
scottschurr added a commit to scottschurr/rippled that referenced this issue Apr 6, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are a
few RPC responses in addition to tooBusy that now have non-OK
HTTP status codes.
scottschurr added a commit to scottschurr/rippled that referenced this issue Apr 8, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are a
few RPC responses in addition to tooBusy that now have non-OK
HTTP status codes.
scottschurr added a commit to scottschurr/rippled that referenced this issue Apr 8, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are a
few RPC responses in addition to tooBusy that now have non-OK
HTTP status codes.
scottschurr added a commit to scottschurr/rippled that referenced this issue Apr 11, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are a
few RPC responses in addition to tooBusy that now have non-OK
HTTP status codes.
scottschurr added a commit to scottschurr/rippled that referenced this issue May 20, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are a
few RPC responses in addition to tooBusy that now have non-OK
HTTP status codes.
scottschurr added a commit to scottschurr/rippled that referenced this issue Aug 4, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are a
few RPC responses in addition to tooBusy that now have non-OK
HTTP status codes.
scottschurr added a commit to scottschurr/rippled that referenced this issue Nov 29, 2022
Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are
quite a number of RPC responses in addition to tooBusy that
now have non-OK HTTP status codes.

The new return HTTP return codes are only enabled by including
"ripplerpc": "3.0" or higher in the original request.
Otherwise the historical value, 200, continues to be returned.
intelliot pushed a commit that referenced this issue Jan 3, 2023
Fixes #4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are
quite a number of RPC responses in addition to tooBusy that
now have non-OK HTTP status codes.

The new return HTTP return codes are only enabled by including
"ripplerpc": "3.0" or higher in the original request.
Otherwise the historical value, 200, continues to be returned.
This ensures that this is not a breaking change.
dangell7 pushed a commit to Transia-RnD/rippled that referenced this issue Jan 24, 2023
…F#4143)

Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are
quite a number of RPC responses in addition to tooBusy that
now have non-OK HTTP status codes.

The new return HTTP return codes are only enabled by including
"ripplerpc": "3.0" or higher in the original request.
Otherwise the historical value, 200, continues to be returned.
This ensures that this is not a breaking change.
dangell7 pushed a commit to Transia-RnD/rippled that referenced this issue Mar 5, 2023
…F#4143)

Fixes XRPLF#4005

Makes it possible for internal RPC Error Codes to associate
themselves with a non-OK (200) HTTP status code.  There are
quite a number of RPC responses in addition to tooBusy that
now have non-OK HTTP status codes.

The new return HTTP return codes are only enabled by including
"ripplerpc": "3.0" or higher in the original request.
Otherwise the historical value, 200, continues to be returned.
This ensures that this is not a breaking change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@intelliot @WietseWind @syeduali93 @cjcobb23 and others