Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions samcli/lib/utils/rate_limiter.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: Missing module-level logger.

Every other non-trivial utility in samcli/lib/utils/ defines LOG = logging.getLogger(__name__). The acquire() method's busy-wait loop is a great candidate for LOG.debug() calls to aid diagnostics (e.g., logging when waiting for token refill).

Thread-safe rate limiter using the token bucket algorithm.

Useful for throttling API calls to AWS services or other external endpoints
to stay within service quotas.
"""

import threading
import time


class RateLimiter:
"""
A token bucket rate limiter.

Parameters
----------
rate : float
Number of tokens added per second.
burst : int
Maximum number of tokens the bucket can hold.

Examples
--------
>>> limiter = RateLimiter(rate=10, burst=10)
>>> limiter.acquire() # blocks until a token is available
>>> limiter.try_acquire() # returns True/False without blocking
"""

def __init__(self, rate: float, burst: int):
if rate <= 0:
raise ValueError("rate must be positive")
if burst <= 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 Nit: burst is typed as int but the <= 0 guard doesn't reject float values like 1.5. A caller passing burst=1.5 silently gets a float bucket capacity. Consider adding if not isinstance(burst, int): raise TypeError(...) to enforce the type hint.

raise ValueError("burst must be a positive integer")

self._rate = float(rate)
self._burst = burst
self._tokens = float(burst)
self._last_refill = time.monotonic()
self._lock = threading.Lock()

def _refill(self):
now = time.monotonic()
elapsed = now - self._last_refill
self._tokens = min(self._burst, self._tokens + elapsed * self._rate)
self._last_refill = now

def try_acquire(self, tokens: int = 1) -> bool:
"""
Try to consume tokens without blocking.

Parameters
----------
tokens : int
Number of tokens to consume.

Returns
-------
bool
True if tokens were acquired, False otherwise.
"""
with self._lock:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: Negative tokens values corrupt the rate limiter state.

Neither try_acquire() nor acquire() validates the tokens parameter. Calling try_acquire(tokens=-1) will always succeed (since self._tokens >= -1 is always true) and then adds a token to the bucket (self._tokens -= -1), silently inflating the token count beyond burst.

Fix: Add validation at the top of both methods:

if tokens <= 0:
    raise ValueError("tokens must be positive")

self._refill()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Major: try_acquire(tokens=N) where N > burst will always return False silently.

If a caller requests more tokens than the bucket can ever hold, this is almost certainly a programming error — but the caller gets no feedback, just an eternal False (or an infinite spin in acquire() without timeout).

Fix: Validate in both methods:

if tokens > self._burst:
    raise ValueError(f"tokens ({tokens}) exceeds burst capacity ({self._burst})")

if self._tokens >= tokens:
self._tokens -= tokens
return True
return False

def acquire(self, tokens: int = 1, timeout: float = None) -> bool:
"""
Block until tokens are available or timeout is reached.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: timeout: float = None should be timeout: Optional[float] = None.

The rest of the samcli codebase (e.g., boto_utils.py, async_utils.py) consistently uses Optional from typing. This also makes the type hint technically correct since None is not a float.

Parameters
----------
tokens : int
Number of tokens to consume.
timeout : float, optional
Maximum seconds to wait. None means wait indefinitely.

Returns
-------
bool
True if tokens were acquired, False if timed out.
"""
deadline = None if timeout is None else time.monotonic() + timeout

while True:
with self._lock:
self._refill()
if self._tokens >= tokens:
self._tokens -= tokens
return True
deficit = tokens - self._tokens
wait_time = deficit / self._rate

if deadline is not None:
remaining = deadline - time.monotonic()
if remaining <= 0:
return False
wait_time = min(wait_time, remaining)

time.sleep(wait_time)
64 changes: 64 additions & 0 deletions tests/unit/lib/utils/test_rate_limiter.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
"""Tests for the rate limiter utility."""

import threading
import time
from unittest import TestCase

from samcli.lib.utils.rate_limiter import RateLimiter


class TestRateLimiter(TestCase):
def test_try_acquire_succeeds_when_tokens_available(self):
limiter = RateLimiter(rate=10, burst=5)
self.assertTrue(limiter.try_acquire())

def test_try_acquire_fails_when_exhausted(self):
limiter = RateLimiter(rate=10, burst=2)
self.assertTrue(limiter.try_acquire())
self.assertTrue(limiter.try_acquire())
self.assertFalse(limiter.try_acquire())

def test_tokens_refill_over_time(self):
limiter = RateLimiter(rate=100, burst=1)
self.assertTrue(limiter.try_acquire())
self.assertFalse(limiter.try_acquire())
time.sleep(0.02) # wait for refill
self.assertTrue(limiter.try_acquire())

def test_acquire_with_timeout_returns_false_on_expiry(self):
limiter = RateLimiter(rate=1, burst=1)
limiter.try_acquire() # drain
self.assertFalse(limiter.acquire(timeout=0.01))

def test_acquire_blocks_until_available(self):
limiter = RateLimiter(rate=100, burst=1)
limiter.try_acquire() # drain
start = time.monotonic()
self.assertTrue(limiter.acquire(timeout=1.0))
elapsed = time.monotonic() - start
self.assertLess(elapsed, 0.5)

def test_invalid_rate_raises(self):
with self.assertRaises(ValueError):
RateLimiter(rate=0, burst=1)

def test_invalid_burst_raises(self):
with self.assertRaises(ValueError):
RateLimiter(rate=1, burst=0)

def test_thread_safety(self):
limiter = RateLimiter(rate=1000, burst=100)
results = []

def worker():
acquired = sum(1 for _ in range(20) if limiter.try_acquire())
results.append(acquired)

threads = [threading.Thread(target=worker) for _ in range(10)]
for t in threads:
t.start()
for t in threads:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Major: Thread-safety test assertion is flawed — will produce false negatives.

With rate=1000 tokens/sec, the bucket refills significantly during test execution (10 threads × 20 calls takes measurable wall time). The total acquired tokens can legitimately exceed the initial burst of 100, causing the assertion sum(results) <= 100 to fail sporadically.

Fix: Use a near-zero refill rate so the bucket doesn't meaningfully refill during the test:

limiter = RateLimiter(rate=0.001, burst=100)

This makes the assertion reliable while still testing thread safety of concurrent try_acquire() calls.

t.join()

# Total acquired should not exceed burst (100)
self.assertLessEqual(sum(results), 100)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: Missing test coverage for edge cases.

Consider adding tests for:

  • try_acquire(tokens=-1) / try_acquire(tokens=0) — should raise ValueError after the fix above
  • try_acquire(tokens=N) where N > burst — should raise ValueError
  • acquire() with tokens > burst — verify it doesn't spin forever
  • Concurrent acquire() calls (only try_acquire() is tested under concurrency)