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

Create a custom throttling error class type. #472

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

jensenb
Copy link
Contributor

@jensenb jensenb commented Nov 22, 2022

I believe throttling errors are bound to affect anyone who uses the library. This change introduces a custom throttling error type that is thrown whenever one of the REST calls returns a 429 status code.

Currently the code simply crashes at unrelated location and is likely confusing for any users. By throwing a custom error type, client code can more easily react to the throttling error, by avoiding any retries for the requested 15 mins.

I should note that is only a small step in the direction of proper error handling, as the _restcall method returns an empty string if any errors are encountered with the exception of timeout error.

@hahn-th
Copy link
Owner

hahn-th commented Nov 23, 2022

Thx for your help.
Your changes does not affect the async calls.
At the moment i am not quite sure if an exception should be thrown or a json string should be returned. Do you have an example of json result of a throttling error?
Do you have discord? Can you get in touch with me? agonist#6159

@jensenb
Copy link
Contributor Author

jensenb commented Nov 23, 2022

There is no json result when the HMIP rest call returns a 429 status code. Because there is no content field returned by the requests lib, the code in connection._restcall then returns an empty string, which the caller code cannot properly handle. It results in a stack trace that looks something like this:

Traceback (most recent call last):
  File "/code/hmip_to_influxdb.py", line 85, in <module>
    write_house_temps(URL, BUCKET, ORG, TOKEN, args.wait_time)
  File "/code/hmip_to_influxdb.py", line 35, in write_house_temps
    home.get_current_state()
  File "/usr/local/lib/python3.9/site-packages/homematicip/home.py", line 292, in get_current_state
    return self.update_home(json_state, clearConfig)
  File "/usr/local/lib/python3.9/site-packages/homematicip/home.py", line 314, in update_home
    self._get_devices(json_state)
  File "/usr/local/lib/python3.9/site-packages/homematicip/home.py", line 349, in _get_devices
    for id_, raw in json_state["devices"].items():
TypeError: string indices must be integers

@jensenb
Copy link
Contributor Author

jensenb commented Nov 23, 2022

I argue that getting a 429 (or any 4XX) response code from the rest call is an exceptional state, so it would make sense to throw an exception when this happens. There is no meaningful response any of the client functions like for example Home.get_state() can return in such situation. Better to inform the user code that something went wrong (like throttling) by raising an exception, and let the user code choose how to deal with it.

@jensenb
Copy link
Contributor Author

jensenb commented Nov 23, 2022

Yes this change only affects the synchronous rest call. I have not yet experiments with web sockets implementation.

I also think it would make sense to move all the custom exception classes to someplace more top level in the package, like

homematicip.exceptions

for example. This would be more in line with most other python packages.

But I didn't want to change too much for an initial PR.

@jensenb
Copy link
Contributor Author

jensenb commented Nov 23, 2022

I just added the exception to the asynchronous web sockets implementation as well.

@jensenb
Copy link
Contributor Author

jensenb commented Nov 23, 2022

One more point I would make as well, why throttling errors should be reflected differently than other error response codes (by having their own exception class): I think that handling any other 4XX error 403 indicates a fundamental problem with the user code or configuration and is unlikely to succeed if retried. A throttling error is likely to succeed if repeated, assuming user code waits long enough.

@hahn-th
Copy link
Owner

hahn-th commented Nov 23, 2022

Thats exactly what i was thinking about. On which errors it is neccessary to repeat a request (with a increasing sleep time). At the end it is timeout and maybe the throttling error. All others are more fundamental problems as you said.

@hahn-th hahn-th merged commit 2099ccc into hahn-th:master Jan 17, 2023
@hahn-th
Copy link
Owner

hahn-th commented Feb 1, 2023

Are you still interrested in refactoring the event handling?
I am working on a version 2.0 and your ideas would be really appreciated.
If yes: homematicip-rest-api@thomas-hahn.org
or on Discord: agonist#6159

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 this pull request may close these issues.

None yet

2 participants