Stop waiting forever when "currency not ready" #136
Conversation
|
When adding a timeout you should be catching ReadTimeout and ConnectTimeout exceptions. Otherwise this is going to change the behaviour of the package. In the event of a timeout, the timeout exception will be thrown out of this function rather than the intended Little bit of info on catching timeouts in requests: https://stackoverflow.com/questions/16511337/correct-way-to-try-except-using-python-requests-module |
Thanks for the comment. Code modified to catch these exceptions and raise RatesNotAvailableError instead |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a timeout to external currency rate requests to avoid indefinite waits and raises a specific error when the service is unavailable.
- Introduce a 5-second timeout on all
requests.getcalls inget_rates,get_rate, andconvert. - Catch
ReadTimeoutandConnectTimeoutto raiseRatesNotAvailableError. - Import the new exceptions from
requests.exceptions.
Comments suppressed due to low confidence (2)
forex_python/converter.py:62
- [nitpick] The error message could be more descriptive (e.g., include 'timeout' or the URL) to aid debugging when this exception is raised.
raise RatesNotAvailableError("Currency Rates Source Not Ready")
forex_python/converter.py:60
- Add or update tests to cover timeout scenarios, ensuring that
RatesNotAvailableErroris raised when the request exceeds the timeout.
response = requests.get(source_url, params=payload, timeout=5)
| @@ -55,7 +56,10 @@ def get_rates(self, base_cur, date_obj=None): | |||
| date_str = self._get_date_string(date_obj) | |||
| payload = {'base': base_cur, 'rtype': 'fpy'} | |||
| source_url = self._source_url() + date_str | |||
| response = requests.get(source_url, params=payload) | |||
| try: | |||
| response = requests.get(source_url, params=payload, timeout=5) | |||
There was a problem hiding this comment.
[nitpick] Consider making the timeout value configurable (e.g., via a module-level constant or initialization parameter) rather than hardcoding 5 seconds.
Copilot uses AI. Check for mistakes.
| response = requests.get(source_url, params=payload) | ||
| try: | ||
| response = requests.get(source_url, params=payload, timeout=5) | ||
| except (ReadTimeout, ConnectTimeout): |
There was a problem hiding this comment.
It may be better to catch the broader requests.exceptions.Timeout base class, which covers both read and connect timeouts in a single handler.
| except (ReadTimeout, ConnectTimeout): | |
| except requests.exceptions.Timeout: |
Copilot uses AI. Check for mistakes.
| @@ -55,7 +56,10 @@ def get_rates(self, base_cur, date_obj=None): | |||
| date_str = self._get_date_string(date_obj) | |||
| payload = {'base': base_cur, 'rtype': 'fpy'} | |||
| source_url = self._source_url() + date_str | |||
| response = requests.get(source_url, params=payload) | |||
| try: | |||
There was a problem hiding this comment.
[nitpick] The try/except block is duplicated across three methods. Extract a helper method (e.g. _safe_get) to reduce duplication and centralize timeout handling.
Copilot uses AI. Check for mistakes.
the problem:
when the url is down, the library waits just about forever for the response that's doomed to fail. i highly suspect this is the cause of all the "currency not ready" issues here. it could literarily be like a minute.
the fix:
wait only 5 seconds instead. (this is just a random number i came up with. feel free to change it to something else that suits your need better.)
detailed explanation of why this is a good thing to do
https://datagy.io/python-requests-timeouts/