Skip to content

Commit

Permalink
Fix IntervalRateLimiter initial state
Browse files Browse the repository at this point in the history
Summary: When using monotonic clocks the epoch may be reset on system reboot, so `0` may not be enough in the past when requesting a large interval to guarantee that the first check succeeds. Switch to the minimum integer instead.

Reviewed By: satyajanga, terrelln

Differential Revision: D51509394

fbshipit-source-id: 8ff1339184f4cfc9a179e24bcfd96229e39b7c57
  • Loading branch information
ot authored and facebook-github-bot committed Nov 22, 2023
1 parent 1a1dfd6 commit fb047ca
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 2 deletions.
2 changes: 1 addition & 1 deletion folly/logging/RateLimiter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ bool IntervalRateLimiter::checkSlow() {
return false;
}

if (ts == 0) {
if (ts == kInitialTimestamp) {
// If we initialized timestamp_ for the very first time increment count_ by
// one instead of setting it to 0. Our original increment made it roll over
// to 0, so other threads may have already incremented it again and passed
Expand Down
11 changes: 10 additions & 1 deletion folly/logging/RateLimiter.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <atomic>
#include <chrono>
#include <cstdint>
#include <type_traits>

#include <folly/Chrono.h>

Expand Down Expand Up @@ -49,6 +50,14 @@ class IntervalRateLimiter {
}

private:
// First check should always succeed, so initial timestamp is at the beginning
// of time.
static_assert(
std::is_signed<clock::rep>::value,
"Need signed time point to represent initial time");
constexpr static auto kInitialTimestamp =
std::numeric_limits<clock::rep>::min();

bool checkSlow();

const uint64_t maxPerInterval_;
Expand All @@ -61,7 +70,7 @@ class IntervalRateLimiter {
// Ideally timestamp_ would be a
// std::atomic<clock::time_point>, but this does not
// work since time_point's constructor is not noexcept
std::atomic<clock::rep> timestamp_{0};
std::atomic<clock::rep> timestamp_{kInitialTimestamp};
};

} // namespace logging
Expand Down
5 changes: 5 additions & 0 deletions folly/logging/test/RateLimiterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,8 @@ TEST(RateLimiter, concurrentThreads) {
// We should have passed the check exactly maxEvents times
EXPECT_EQ(maxEvents, count.load(std::memory_order_relaxed));
}

TEST(RateLimiter, LargeInterval) {
IntervalRateLimiter limiter{1, std::chrono::years{1}};
EXPECT_TRUE(limiter.check());
}

0 comments on commit fb047ca

Please sign in to comment.