remove refresh throttle

[mkmer]

Description:

After a bit of research into why HA wasn't updating the new switch platform (for camera motion detection), I found the throttle was poping force preventing the refresh from seeing "force". Since refresh already has a built in limiter, there is no need to put throttle in front.
We could just do this and it will solve the alarm control panel issue as well.

Related issue (if applicable): fixes #

Checklist:

• Local tests with tox run successfully PR cannot be meged unless tests pass
• Changes tested locally to ensure platform still works as intended
• Tests added to verify new code works

[fronzbot]

Are you positive this is the root cause? The Throttle decorator will see the force keyword given to refresh and so Throttle gets ignored when given. We even have tests to ensure this is working properly: https://github.com/fronzbot/blinkpy/blob/dev/tests/test_util.py#L30

The reason this was added was because some instances in home-assistant would call refresh with force_cache=True multiple times and the users accounts were getting flagged so I'm hesitant to remove it

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (32f3235) 99.86% compared to head (d3f1e22) 99.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #809      +/-   ##
==========================================
- Coverage   99.86%   99.86%   -0.01%     
==========================================
  Files           8        8              
  Lines        1454     1453       -1     
==========================================
- Hits         1452     1451       -1     
  Misses          2        2              

Flag	Coverage Δ
unittests	99.86% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mkmer]

Well... the decorator is removing the force=true which is preventing the updates at service calls in HA (with force=true). From updating.
This change 2b07d62 is a way around the "standard" refresh in the API, but I can't figure out how to do this same change for the motion switch and for updating the pictures after a snap. I had the refresh code in HA but it wasn't working - The decorator is the reason.

I suppose we could refresh in the API (if you can help me figure out how to do it in the camera.py module) or remove the decorator so that HA can do it...

[fronzbot]

The issue is the Throttle decorator is throttling refresh to 2s. If removing that is fixing the problem, then the HA integration is calling refresh much too frequently and will almost certainly in accounts getting banned on Blink servers.

As long as we have some sort of mechanism to ensure the integration can't spam refresh, that's fine. But I believe getting rid of the Throttle decorator removes that mechanism. So your alternative solution of removing it here and putting the burden on HA to solve it makes sense to me. Philosophically, we probably shouldn't be solving HA problems in this library anyways. Only reason I added that here originally was because...well...you see how long it takes to get PRs merged sometimes 😄

[mkmer]

I'm not partial to this solution. It just took me a bit to figure out what the heck happened...
The service calls will still be throttled ex:

blinkpy/blinkpy/api.py

Lines 133 to 135 in daea3cf

	@Throttle(seconds=MIN_THROTTLE_TIME)
	async def request_system_arm(blink, network):
	"""

So maybe we should add local refresh for these calls that are not limited?
What I believe is happening (based on some tracing)- we call an arm/disarm, then immediately try to refresh and it's throttled.
When force = true, the throttle decorator POPs force (which I added awhile back because every function except refresh do not except force in the args) and then refresh doesn't see force.
The solution we had for ARM/DISARM could be added to each of these calls too, and then leave the decorator.

We probably only need to call a subset of

blinkpy/blinkpy/camera.py

Line 218 in daea3cf

async def update(self, config, force_cache=False, expire_clips=True, **kwargs):

but it's not clear to me what/how to do (mental block?)

Besides, maybe I'm reading this wrong, but refresh already has a limit built in doesn't it?

blinkpy/blinkpy/blinkpy.py

Line 90 in daea3cf

if force or force_cache or self.check_if_ok_to_update():

check_if_ok_to_update - unless forced... Does it really need a throttle too?

I am not nearly as close to these functions and how the operate as your are.

[fronzbot]

The service calls will still be throttled ex:

So maybe we should add local refresh for these calls that are not limited?

Besides, maybe I'm reading this wrong, but refresh already has a limit built in doesn't it?

I am not nearly as close to these functions and how the operate as your are.

So all of this comes down to some historical context. This library was built for home-assistant, but not exclusively used by home-assistant. There's a decent amount of people that use it just as a python library and have their own scripts for arming/taking pics/etc. From the perspective of home-assistant: yes, we could just add throttles around API calls and be fine but this got annoying from a scripting perspective where you're maybe making a bunch of API calls all at once, but you only do it once (rather than HA which is constantly trying to refresh info). So I opted for a belt-and-suspenders approach of having a constant refresh time, but the ability to force to give flexibility. This is perfect for command line and then was also very useful for home-assistant users who would just spam blink.refresh in their automations or scripts.

Ultimately, though, I think you're right that adding throttles to the individual API calls vs the refresh makes the most sense. We would just have to make sure we add them in the right places and do a comparison between the logs to make sure all the logs are appropriately caught (probably a test for this would be best- write it with the existing lib and then make the modifications and make sure nothing breaks)

[mkmer]

Thanks for the context. As I am discovering even with HA, there are many various configurations that I had not tested.
I think we already have throttles on the API calls.

blinkpy/blinkpy/api.py

Lines 133 to 135 in daea3cf

	@Throttle(seconds=MIN_THROTTLE_TIME)
	async def request_system_arm(blink, network):
	"""

blinkpy/blinkpy/api.py

Lines 150 to 153 in daea3cf

	@Throttle(seconds=MIN_THROTTLE_TIME)
	async def request_system_disarm(blink, network):
	"""
	Disarm system.

blinkpy/blinkpy/api.py

Lines 179 to 183 in daea3cf

	@Throttle(seconds=MIN_THROTTLE_TIME)
	async def request_homescreen(blink):
	"""Request homescreen info."""
	url = f"{blink.urls.base_url}/api/v3/accounts/{blink.account_id}/homescreen"
	return await http_get(blink, url)

etc...
These are why I needed to change to POP the force value - otherwise these functions were passed force and failed to execute. Refresh on the other had takes the force parameter - so now that the Throttle popped the value, it doesn't make it to the refresh function. Besides, refresh has a built in check for "throttling" self.check_if_ok_to_update().

blinkpy/blinkpy/blinkpy.py

Line 90 in daea3cf

if force or force_cache or self.check_if_ok_to_update():

On the good side, the fellow over on the HA thread removed the throttle ONLY and it solved his arm system delay issue. I think it's safe to remove Throttle from this function only essentially leaving the API's throttled on one value, and the refresh function throttled on it's own value, assuming I understood what you've said above :)

[fronzbot]

On the good side, the fellow over on the HA thread removed the throttle ONLY and it solved his arm system delay issue.

Yeah I know it will fix it but what I'm worried about are events where API calls are triggered too quickly via automations, etc which that testing wouldn't catch. It's more me worrying that I didn't implement throttling on all the correct API calls that refresh calls

So what I need to do (just haven't had time) is go through the refresh call and trace every API call that could be made and make sure throttling is on all the right ones. Right now my placement of that throttle decorator is a bit haphazard so I'm not totally convinced I have it all covered (hence me just throwing it around refresh and calling it a day).

Re: built in throttle- the issue is when force is used I wanted to prevent people from being able to rapidly trigger API calls. So wrapping it around refresh ensured I hit everything and didn't miss a call. It was a "I have a hammer and everything looks like a nail" and I never revisited it, so I'm just being a bit cautious as a result. Logically I agree with everything you said, but I wrote the code so I trust none of it lol

[mkmer]

Oh believe me, I compare code to a "mold growth experiment" - put it in the dark and let it grow, see what happens :)
My senior colleagues compare software to a gas. It expands to fill all available space :)

On that note, I worked on moving to a switch platform for the motion detection function on the HA side, and this change also addressed the update problem in that as well.
home-assistant/core#102789

I'm hoping we have a solution in place for the 2023.12 release, but understand time is precious.

[fronzbot]

I started going through the code and there are a ton of API calls not throttled that were covered by the refresh catch-all. If we add @Throttle to those calls instead of refresh I think we run into the same issue you're trying to solve.

I think we need to remove the refresh pop from @Throttle and instead add kwarg processing to the API calls you were having issues with such that force is a valid argument, just unprocessed by the API call. Something like:

@Throttle(seconds=MIN_THROTTLE_TIME)
async def request_motion_detection_enable(blink, network, camera_id, **kwargs):

While going back to this original implementation: 416b275

[mkmer]

We can't really remove the pop: if you try to call any of the throttled functions they are not expecting a force argument and fail (all of the throttled components will fail the tests confirming force works). We could call refresh with force in the args twice - maybe that will do it?
As a point of interest, so far, the fellow on HA issue as well as myself are running this "fix" without any issues or throttling. I think we're "ok" taking it out.

[fronzbot]

We can't really remove the pop: if you try to call any of the throttled functions they are not expecting a force argument and fail

Yeah my suggestion is to add **kwargs as an argument so that those methods can receive any arbitrary keyword argument without failing. Does that not work?

[fronzbot]

As a point of interest, so far, the fellow on HA issue as well as myself are running this "fix" without any issues or throttling. I think we're "ok" taking it out.

Given the history of Blink cutting off access prior to having the throttling in the library, I don't think this is a large enough sample size to confirm it's working reliably and think it's safer to retain the previous functionality

[mkmer]

Maybe something like this will work better now that we have asyncio??
dev...mkmer:blinkpy:async-wrapper

[fronzbot]

Ooo that's interesting. I think that gives us the best of both worlds, right?

Only question I'd have (related to asyncio) is what if I make like 1000 API calls within the throttle window? How does asyncio handle that? I have very little intuition for the library. (And I'm not saying that's a scenario we would want to plan for- I'd chalk that one up to user error. Just trying to understand where the limits are)

[mkmer]

Well, I was thinking this would holdup the "current" call until the throttle was finished then just make the call (which it does). This in theory would prevent a second call as it held up the flow of the application thread.

If we call it concurrently, the second call is supposed to be delayed by now + 2*delta. I don't think this code achieves that. We may need it to do that too (I'm not sure).

At this point, I think we're find removing the throttle decorator from refresh - maybe this is something in our back pocket if it breaks things for the general population...

[mkmer]

On second review, I think my proposed wrapper is broken - throttle_method needs to be awaited and it is not.
It's an "idea" more than functional code at this point.

[mkmer]

Digging into the other PR #823, we do throttle the lower level homescreen call - so removing the upper level throttle is not blocking the api throttle (and the force parameter is not passed down either).