Katrix / AckCord

A Discord library for Scala using Akka
https://ackcord.katsstuff.net/
MIT License
113 stars 15 forks source link

Rate Limiter never releases once rate limit is hit #28

Closed jonathan-ostrander closed 4 years ago

jonathan-ostrander commented 4 years ago

AckCord Version: 0.16.1

This issue is occurring through the use of Requests#singleFuture so it's possible that there is just a bug with how rate limits get updated through this flow. I'm still trying to get acquainted with the code and debug the issue.

Expected behavior:

Rate limit is hit for a specific bucket. All requests for the bucket are queued and then sent in order when the rate limit times out.

Actual behavior:

Rate limit is hit for a specific bucket. All subsequent requests for that bucket are queued but never sent as long as the same Ratelimiter actor lives.

Katrix commented 4 years ago

Does code like this work fine for you?

jonathan-ostrander commented 4 years ago

It looks like singleFuture goes through the same code path, but let me try to refactor my code to use that pattern instead.

Katrix commented 4 years ago

Can't reproduce with a for loop, singleFuture and withSideEffects either.

Would be great if you could post the code that's causing this

jonathan-ostrander commented 4 years ago

Sorry it's a bit dense, but it happens in this logic with creating reactions: https://github.com/jonathan-ostrander/music-quiz/blob/master/src/main/scala/dev/ostrander/musicquiz/actor/Game.scala#L148-L151

jonathan-ostrander commented 4 years ago

I can confirm that rate limiting works when just sending messages, but I see the same behavior with this simplified example when sending both reactions and messages through singleIgnore.

Katrix commented 4 years ago

Hmm, can you confirm that it's actually the fact that the rate limit is never released?

For the above example, it's very likely that it will fail for some of the messages if AckCord has no ratelimit info at all. This is usually the case just after the bot has started. What happens if you try the ratelimit command a second time? Does it work then?

If it does, and you need to send requests in rapid succession like this at the start of the bot's lifespan, take a look at Requests.RequestProperties.retry

jonathan-ostrander commented 4 years ago

Here's an example debug log for the rate limiter for when this happens. It looks like it's updating/resetting the limits fine and then just stops all of a sudden. I can probably take a deeper look into what's going on. My suspicion is that maybe there's a response that doesn't include the reset info in the rate limit response?

jonathan-ostrander commented 4 years ago

Ok, I think I found the issue and I think it's due to the non-relative TilReset path. In one example I found where this was happening, the last headers that were received by my app for rate limits on the reaction path were

12:46:38.628 [AckCord-akka.actor.default-dispatcher-20] DEBUG akka.actor.typed.ActorSystem -
x-ratelimit-bucket: d6b7697c78814ecd72bb20df05517c78
x-ratelimit-limit: 1
x-ratelimit-remaining: 0
x-ratelimit-reset: 1596818798.567
x-ratelimit-reset-after: 0.001

That reset value corresponds to 12:46:38.567 which is before the call to System.currentTimeMillis() so the value for tilReset will be negative (since the system time is later than the reset time and isValid will be false so the updater is never called. Should be a simple fix.