Add rate limiter#383
Conversation
ol-iver
left a comment
ol-iver
left a comment
There was a problem hiding this comment.
Thanks for the rate limiter. It's nice 😄
Please add a switch to enable/disable the rate limiter for a receiver instance in general.
| if skip_confirmation: | ||
| task = asyncio.create_task( | ||
| self.httpx_async_client.async_get( | ||
| endpoint, | ||
| self.host, | ||
| self.timeout, | ||
| self.read_timeout, | ||
| cache_id=cache_id, | ||
| record_latency=False, | ||
| skip_rate_limiter=skip_rate_limiter, | ||
| ) | ||
| ) | ||
| task.add_done_callback( | ||
| self._http_callback_tasks.discard | ||
| ) # Prevent garbage collection | ||
| return httpx.Response(200, text="") | ||
|
|
There was a problem hiding this comment.
Creating a task for each HTTP request is too much compute overhead and faking the response is no good practice. Please remove the skip_confirmation part for HTTP requests.
There was a problem hiding this comment.
Fair enough, I don't think it's actually needed anyway. From what I remember it was a "red herring", and the delay I saw came from something else.
| initial_wait_ms: float = 100.0, | ||
| min_wait_ms: float = 100.0, | ||
| max_wait_ms: float = 200.0, |
There was a problem hiding this comment.
Why do you call this ..._wait_ms? This gives the impression that even the first request needs to wait 100ms.
Later you even calculate rates from this wait times. Please call it rate from the beginning.
Also the minimum rate (10 req/s) looks too low. It could be 100 req/s.
There was a problem hiding this comment.
I landed on 10 rps by sending volume up/down commands with different delays in-between, and landed on 10 because that seemed like a good balance with speed for the command without causing the receiver to buffer the commands and then keep executing them for seconds after I stopped sending commands. My thought was that it is more irritating to not be able to send a different command after you let go of a button than having volume not increase by more than 10 steps per second. I'm open for adjusting the 10rps, but 100 will cause issues.
| if record_latency: | ||
| self._rate_limiter.record_latency(self.host, start) |
There was a problem hiding this comment.
You could move this from finally to else.
The default timeout is 2s. When the receiver does not send a confirmation within this timeframe, the command might never confirm. This should not affect the rate limit.
There was a problem hiding this comment.
Fixed, also found another place where it applies
| async def aclose(self): | ||
| """Close rate limiter when done.""" | ||
| await self.rate_limiter.aclose() |
There was a problem hiding this comment.
This method isn't called anywhere.
There was a problem hiding this comment.
Yes, I think this one slipped through, it is called in my fork in code that has not been included here. Do you want me to delete it or replace it with something else so that the resource is disposed when no longer needed?
| if latency_tracker := self._latencies.get(destination): | ||
| latency_tracker.update(seconds) | ||
|
|
||
| async def aclose(self) -> None: |
There was a problem hiding this comment.
The only place where this method is called, is never called 😉
There must be a better way to do this.
There was a problem hiding this comment.
Seems my comment disappeared, it is a leftover from my own fork that does things differently. Fixed it (I hope)
| while not self._shutdown_event.is_set(): | ||
| await asyncio.sleep(self._adjust_interval) | ||
| if self._shutdown_event.is_set(): | ||
| break | ||
| avg = self._latencies[key].value | ||
| new_rate = self._target_rate(avg) | ||
| # AsyncLimiter does not expose direct rate mutation; | ||
| # recreate it atomically under a lock to avoid races. | ||
| async with self._locks[key]: | ||
| self._limiters[key] = AsyncLimiter(new_rate, time_period=1) |
There was a problem hiding this comment.
Why do you recreate the limiter every 2 seconds even when there are no changes? This does not look efficient.
You could start a task adjust the rate limiter in record_latency if the current avg rate and the limiter rate differ by more than 20% and maximum once every 10s.
Then you can also remove the update loop which cannot be stopped.
| skip_rate_limiter=True, | ||
| record_latency=False, |
There was a problem hiding this comment.
Many commands hammer on the receiver in a short period of time. Isn't this the perfect place to use rate limiting?
There was a problem hiding this comment.
This is yet another case of mismatch between this repo and my fork, in my work I have a delay between each command so the rate limiter is not needed there and I didn't want the automatic adjust of the rate limiter to be affected since I did do my own measurements to make sure it is as quick as possible without stalling the receiver. I will change it to work with your repo.
| seconds = time.monotonic() - start | ||
| # Only record if destination already initialized | ||
| if latency_tracker := self._latencies.get(destination): | ||
| latency_tracker.update(seconds) |
There was a problem hiding this comment.
Could you skip the update for big values like >1s.
The audyssey endpoint of my receiver take like 4 seconds to respond and I don't want the rate limit go down just because I call it a couple of times.
There was a problem hiding this comment.
I think 1s might be too aggressive and might sidestep the intent of the limiter. I'll exclude those that take 2s. Let me know if you disagree and I can change it to 1.
| ) | ||
| start = time.monotonic() | ||
| if not skip_rate_limiter: | ||
| await self._rate_limiter.acquire(self.host) |
There was a problem hiding this comment.
Please use a different limiter for telnet as HTTP and telnet are also different endpoints.
There was a problem hiding this comment.
They are different, _rate_limiter is constructed by attr in the DenonAVRTelnetApi and HTTPXAsyncClient, or are you referring to using different settings?
| data=data, | ||
| cache_id=cache_id, | ||
| record_latency=False, | ||
| skip_rate_limiter=True, |
There was a problem hiding this comment.
Why is this method not rate limited?
In my opinion all requests should go though the rate limiter if it is enabled.
There was a problem hiding this comment.
Also because of how the code is used in my fork, but you're right, in a general purpose library it makes sense to rate limit everything
…eature/rate_limiter # Conflicts: # requirements.txt
2 participants
First of many PRs to come :)
Relates to #375