-
Notifications
You must be signed in to change notification settings - Fork 286
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
limit: Fix rate limit not reseting state correctly #438
Conversation
Signed-off-by: Lucio Franco <[email protected]>
All tests pass locally I am not sure why we aren't seeing tests in here. |
Signed-off-by: Lucio Franco <[email protected]>
@@ -65,7 +65,9 @@ where | |||
match self.state { | |||
State::Ready { .. } => return self.inner.poll_ready().map_err(Into::into), | |||
State::Limited(ref mut sleep) => { | |||
try_ready!(sleep.poll()); | |||
if let Async::NotReady = sleep.poll()? { | |||
tracing::trace!("rate limited exceeded; sleeping."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukesteensen I like this better, what do you think? This should be the only case where we apply back pressure in this layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little late on this review, but I think this actually broke the whole thing 😄
Before, try_ready!
would return NotReady
if we still needed to sleep. Now we're just checking if we should still be sleeping, logging that we should, and continuing on and marking ourselves Ready
.
I am also unsure how to actually test this behavior, technically speaking the only issue here is extra logging. The actual behavior was correct. This is because we would falsely transition into the limited state but on the next call when we poll the timer its always elapsed. Then the state would get reset correctly. |
We were running into an issue in
Vector
where the rate limit remaining count never got reset and we always hit the branch that sets the limited state. This was because we were double over writing the state. In the first if statement we wouldself.state = State::Ready { .. }
but then we would go to the next if statement and if the remaining was larger than 1 we would reset the state back to the original status with one more remaining subtracted. This would then override the fact that we were resetting the state in the previous if statement. This would then cause us to only reset the state in the poll_ready where we now would have to check the timer. This result of this is that we were probably rate limiting more than expected and logging the trace log much more than expected.cc @lukesteensen
Signed-off-by: Lucio Franco [email protected]