-
Notifications
You must be signed in to change notification settings - Fork 26
Add retries #16
base: master
Are you sure you want to change the base?
Add retries #16
Changes from all commits
d8ed3ec
338b722
0e0d8c2
1b33227
9395cbb
2c85592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,13 @@ | ||
| import base64 | ||
| import json | ||
| from typing import Union, Tuple, Any | ||
| import logging | ||
| import time | ||
| from typing import Union, Tuple, Any, Optional | ||
|
|
||
| import requests | ||
| import urllib3.util | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| TimeoutType = Union[float, Tuple[float, float]] | ||
| """ The type used as "timeout" argument when sending requests. Quantities are in seconds. | ||
|
|
@@ -18,28 +23,104 @@ class FinalityTypes: | |
|
|
||
|
|
||
| class JsonProviderError(Exception): | ||
| pass | ||
| def get_type(self) -> Optional[str]: | ||
| try: | ||
| return self.args[0]["name"] | ||
| except (IndexError, KeyError): | ||
| return None | ||
|
|
||
| def get_cause(self) -> Optional[str]: | ||
| try: | ||
| return self.args[0]["cause"]["name"] | ||
| except (IndexError, KeyError): | ||
| return None | ||
|
|
||
| def is_invalid_nonce_tx_error(self) -> bool: | ||
| try: | ||
| return ( | ||
| self.get_type() == "HANDLER_ERROR" | ||
| and self.get_cause() == "INVALID_TRANSACTION" | ||
| and "InvalidNonce" in self.args[0]["data"]["TxExecutionError"]["InvalidTxError"] | ||
| ) | ||
| except (IndexError, KeyError): | ||
| return False | ||
|
|
||
| def is_expired_tx_error(self) -> bool: | ||
| try: | ||
| return ( | ||
| self.get_type() == "HANDLER_ERROR" | ||
| and self.get_cause() == "INVALID_TRANSACTION" | ||
| and self.args[0]["data"]["TxExecutionError"]["InvalidTxError"] == "Expired" | ||
| ) | ||
| except (IndexError, KeyError): | ||
| return False | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it would probably be better to do the same thing as near-api-js does, but I gave up the moment I looked at code responsible for that (traversing whole returned object tree and comparing with json scheme just to get the error name?) I don't feel competent enough to implement this in python since I've found nothing about this in docs. Although If you think it would be much better to use this json schema in python as well (and maybe can point me at some docs or example error responses) I can implement this here. [1] https://github.com/near/near-api-js/blob/master/packages/near-api-js/src/utils/rpc_errors.ts#L55 |
||
|
|
||
|
|
||
| class JsonProvider(object): | ||
| def __init__(self, rpc_addr, proxies=None): | ||
| def __init__( | ||
| self, | ||
| rpc_addr, | ||
| proxies=None, | ||
| tx_timeout_retry_number: int = 12, | ||
| tx_timeout_retry_backoff_factor: float = 1.5, | ||
| http_retry_number: int = 10, | ||
| http_retry_backoff_factor: float = 1.5, | ||
| session: Optional[requests.Session] = None, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this argument ever used? I’d rather we get rid of it. With it, the prototype of this constructor is a bit weird. If session is given, majority of the arguments are just ignored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you prefer to not use built-in retry mechanism then it's not very useful at the moment. I'll remove it from this PR. The main reason for adding this here was to make it possible to configure HTTP connection pooling(*) which is very useful when doing more than couple of requests to near api per second. I intended to do something like: I'll remove this argument here and create a separate PR which will allow me to do: under the hood it will create session with relevant pool size and (*) relevant docs: https://requests.readthedocs.io/en/latest/api/#requests.adapters.HTTPAdapter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean it’s fine to have optional session argument. My issue was about having mutually exclusive set of arguments. |
||
| ) -> None: | ||
| if isinstance(rpc_addr, tuple): | ||
| self._rpc_addr = "http://%s:%s" % rpc_addr | ||
| else: | ||
| self._rpc_addr = rpc_addr | ||
| self.proxies = proxies | ||
|
|
||
| if session is None: | ||
| # Loosely based on https://findwork.dev/blog/advanced-usage-python-requests-timeouts-retries-hooks/ | ||
| adapter = requests.adapters.HTTPAdapter( | ||
| max_retries=urllib3.util.Retry( | ||
| total=http_retry_number, | ||
| backoff_factor=http_retry_backoff_factor, | ||
| status_forcelist=[502, 503], | ||
| allowed_methods=["GET", "POST"], | ||
| ), | ||
| ) | ||
|
|
||
| session = requests.Session() | ||
| session.mount("https://", adapter) | ||
| session.mount("http://", adapter) | ||
|
Comment on lines
+76
to
+89
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used sessions instead of manual retrying because a built-in mechanism seem more straightforward to me and it allows great customisation (user can pass their own Session object). If you prefer I can change it to manual exception catching and handling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit specious to me. There are now three nested retry loops. We have
It may be somewhat contrived, but in pathological case we may end up retrying 1200 times. I’d say we should at least get rid of the Retry adapter and to the looping logic for 5xx errors in json_rpc. Something like: while True:
try:
return self._json_rpc_once(method, params, timeout)
except HTTPError as e: # whatever the exception type is
if attempt >= self._tx_timeout_retry_number:
raise
if e.status_code // 100 == 5:
log.warning("Retrying request to %s as it returned server error: %s", method, e)
else:
raise
except JsonProviderError as e:
if attempt >= self._tx_timeout_retry_number:
raise
if e.get_type() == "HANDLER_ERROR" and e.get_cause() == "TIMEOUT_ERROR":
log.warning("Retrying request to %s as it has timed out: %s", method, e)
else:
raise
attempt += 1
time.sleep(self._tx_timeout_retry_backoff_factor ** attempt * 0.5)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's what is done by near-api-js ;p Ok, I'll refactor this to have only two levels of retries There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I’ve noticed. I’d argue it’s a bug in JS implementation. |
||
|
|
||
| self._session = session | ||
|
|
||
| self._tx_timeout_retry_number = tx_timeout_retry_number | ||
| self._tx_timeout_retry_backoff_factor = tx_timeout_retry_backoff_factor | ||
|
|
||
| def rpc_addr(self) -> str: | ||
| return self._rpc_addr | ||
|
|
||
| def json_rpc(self, method: str, params: Union[dict, list, str], timeout: 'TimeoutType' = 2.0) -> dict: | ||
| attempt = 0 | ||
| while True: | ||
| try: | ||
| return self._json_rpc_once(method, params, timeout) | ||
| except JsonProviderError as e: | ||
| if attempt >= self._tx_timeout_retry_number: | ||
| raise | ||
| attempt += 1 | ||
|
|
||
| if e.get_type() == "HANDLER_ERROR" and e.get_cause() == "TIMEOUT_ERROR": | ||
| log.warning("Retrying request to %s as it has timed out: %s", method, e) | ||
| else: | ||
| raise | ||
|
|
||
| time.sleep(self._tx_timeout_retry_backoff_factor ** attempt) | ||
|
|
||
| def _json_rpc_once(self, method: str, params: Union[dict, list, str], timeout: 'TimeoutType' = 2.0) -> dict: | ||
| j = { | ||
| 'method': method, | ||
| 'params': params, | ||
| 'id': "dontcare", | ||
| 'jsonrpc': "2.0" | ||
| } | ||
| r = requests.post(self.rpc_addr(), json=j, timeout=timeout, proxies=self.proxies) | ||
| r = self._session.post(self.rpc_addr(), json=j, timeout=timeout, proxies=self.proxies) | ||
| r.raise_for_status() | ||
| content = json.loads(r.content) | ||
| if "error" in content: | ||
|
|
@@ -56,7 +137,7 @@ def send_tx_and_wait(self, signed_tx: bytes, timeout: 'TimeoutType') -> dict: | |
| timeout=timeout) | ||
|
|
||
| def get_status(self, timeout: 'TimeoutType' = 2.0) -> dict: | ||
| r = requests.get("%s/status" % self.rpc_addr(), timeout=timeout) | ||
| r = self._session.get("%s/status" % self.rpc_addr(), timeout=timeout) | ||
|
Comment on lines
-59
to
+140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should go through the retry loop as well. Currently this only retries on server errors but I don’t see why we wouldn’t want to retry on timeouts if we’re retrying other requests on timeout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean the same But yeah, since we'll be handling all timeouts ourselves then retry code might be the same and we can add handling of these error here as well :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. It’s a different timeout that what I was thinking of. What happens if requsets.get’s timeout runs out? Also, at this point maybe we should move TIMEOUT_ERROR handling to _sign_and_submit_tx? JsonProvider would handle 5xx and Account would handle TIMEOUT_ERROR and any other errors of this kind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess that requests's retry logic triggers and once number of attempts is exceeded an exception like requests.Timeout is thrown.
Sounds great, I'll also be able to stick with built-in mechanisms for network/http error retries. |
||
| r.raise_for_status() | ||
| return json.loads(r.content) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied these default number of retries from near-api-js, that's why they're so huge... (~30 retries in total with delay from 1.5s to 1min, theoretically a request can take ~3 minutes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current code request can take up to 6 minutes. Note that JS code multiplies delay by 500 milliseconds which is not done here on line 63.