From 8d20b78c632fbe03b36849d7909147c85643957d Mon Sep 17 00:00:00 2001 From: Louis Mandelstam Date: Tue, 23 Sep 2025 06:37:10 +0200 Subject: [PATCH 1/3] Fix call tracking for all acquisition methods --- src/easylimit/rate_limiter.py | 6 ++++++ tests/test_async_rate_limiter.py | 19 +++++++++++++++++++ tests/test_call_tracking.py | 22 ++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/easylimit/rate_limiter.py b/src/easylimit/rate_limiter.py index 98f0b3a..b5aaed8 100644 --- a/src/easylimit/rate_limiter.py +++ b/src/easylimit/rate_limiter.py @@ -252,6 +252,8 @@ def try_acquire(self) -> bool: self._refill_tokens() if self.tokens >= 1: self.tokens -= 1 + if self._track_calls: + self._record_call(0.0) return True return False @@ -398,6 +400,8 @@ def _try_consume_one_token_sync(self, start_time: float, timeout: Optional[float self._refill_tokens() if self.tokens >= 1: self.tokens -= 1 + if self._track_calls: + self._record_call(time.time() - start_time) return True, 0.0, False if timeout is not None and (time.time() - start_time) >= timeout: return False, 0.0, True @@ -410,6 +414,8 @@ def _try_acquire_sync(self) -> bool: self._refill_tokens() if self.tokens >= 1: self.tokens -= 1 + if self._track_calls: + self._record_call(0.0) return True return False diff --git a/tests/test_async_rate_limiter.py b/tests/test_async_rate_limiter.py index 13095d2..ad8b3dc 100644 --- a/tests/test_async_rate_limiter.py +++ b/tests/test_async_rate_limiter.py @@ -77,6 +77,25 @@ async def test_async_call_tracking(self) -> None: assert stats.total_calls == 3 assert stats.average_delay_seconds >= 0.0 + async def test_async_acquire_records_tracking(self) -> None: + """Direct async_acquire() should increment the tracked call count.""" + limiter = RateLimiter(limit=2, track_calls=True) + + assert limiter.call_count == 0 + assert await limiter.async_acquire() is True + assert limiter.call_count == 1 + + async def test_async_try_acquire_records_tracking(self) -> None: + """async_try_acquire() should only count successful acquisitions.""" + limiter = RateLimiter(limit=1, track_calls=True) + + assert await limiter.async_try_acquire() is True + assert limiter.call_count == 1 + + # Subsequent call has no tokens available yet + assert await limiter.async_try_acquire() is False + assert limiter.call_count == 1 + class TestMixedSyncAsync: """Test mixed sync and async usage to ensure unified locking works.""" diff --git a/tests/test_call_tracking.py b/tests/test_call_tracking.py index 0535f3b..561e82b 100644 --- a/tests/test_call_tracking.py +++ b/tests/test_call_tracking.py @@ -85,6 +85,28 @@ def worker() -> None: assert limiter.call_count == 15 + def test_call_count_increments_for_acquire(self) -> None: + """Call tracking should include direct acquire() usage.""" + limiter = RateLimiter(limit=5, track_calls=True) + + assert limiter.call_count == 0 + assert limiter.acquire() is True + assert limiter.call_count == 1 + + def test_call_count_increments_for_try_acquire(self) -> None: + """Call tracking should include try_acquire() successes only.""" + limiter = RateLimiter(limit=2, track_calls=True) + + assert limiter.try_acquire() is True + assert limiter.call_count == 1 + + assert limiter.try_acquire() is True + assert limiter.call_count == 2 + + # Bucket is empty now; failure should not increment the counter + assert limiter.try_acquire() is False + assert limiter.call_count == 2 + def test_reset_call_count(self) -> None: """Test resetting call count.""" limiter = RateLimiter(limit=5, track_calls=True) From 8e452f46b215e50f1f8352a2203de1cf268b565c Mon Sep 17 00:00:00 2001 From: Louis Mandelstam Date: Fri, 7 Nov 2025 18:56:00 +0200 Subject: [PATCH 2/3] fix: ensure all acquisition methods track calls consistently Fixes call tracking bugs where try_acquire(), async_try_acquire(), and acquire() did not increment call_count when track_calls=True. Changes: - Move call tracking to internal helper methods (_try_consume_one_token_sync, _try_acquire_sync) for cleaner architecture - Remove duplicate tracking from async_acquire (was causing double-counting after rebase with v0.3.2) - Context managers now delegate tracking to underlying acquisition methods - All acquisition methods now consistently track calls Fixes #15 --- CHANGELOG.md | 11 +++++++++++ src/easylimit/__init__.py | 2 +- src/easylimit/rate_limiter.py | 3 --- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8ae7a8..5dd6efb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.3.4] - 2025-11-07 + +### Fixed +- Fixed call tracking bug where `try_acquire()` and `async_try_acquire()` did not increment `call_count` when `track_calls=True` +- Fixed call tracking bug where `acquire()` did not increment `call_count` when called directly (outside context manager) with `track_calls=True` +- All acquisition methods now consistently track calls when tracking is enabled + +### Changed +- Refactored call tracking to be implemented at the internal method level (`_try_consume_one_token_sync`, `_try_acquire_sync`) for cleaner architecture +- Context managers (`__enter__`, `__aenter__`) now delegate tracking to underlying acquisition methods + ## [0.3.3] - 2025-11-07 ### Added diff --git a/src/easylimit/__init__.py b/src/easylimit/__init__.py index 9e925c0..7ae1487 100644 --- a/src/easylimit/__init__.py +++ b/src/easylimit/__init__.py @@ -7,5 +7,5 @@ from .rate_limiter import CallStats, RateLimiter -__version__ = "0.3.3" +__version__ = "0.3.4" __all__ = ["RateLimiter", "CallStats"] diff --git a/src/easylimit/rate_limiter.py b/src/easylimit/rate_limiter.py index b5aaed8..fb3d791 100644 --- a/src/easylimit/rate_limiter.py +++ b/src/easylimit/rate_limiter.py @@ -444,9 +444,6 @@ async def async_acquire(self, timeout: Optional[float] = None) -> bool: while True: acquired, sleep_time, timed_out = await _to_thread(self._try_consume_one_token_sync, start_time, timeout) if acquired: - if self._track_calls: - delay = time.time() - start_time - await _to_thread(self._record_call, delay) return True if timed_out: return False From 2215b0e8fafb8971acaa0e81e56d266689cf00de Mon Sep 17 00:00:00 2001 From: Louis Mandelstam Date: Fri, 7 Nov 2025 19:03:57 +0200 Subject: [PATCH 3/3] refactor: remove inefficient double-locking from _record_call The _record_call() method was acquiring self.lock even though all callers already held the lock. Since self.lock is an RLock (reentrant), this did not cause deadlocks but was inefficient and architecturally unclear. Changes: - Remove lock acquisition from _record_call() - Update docstring to document that caller must hold lock - Add Args documentation for clarity All 148 tests still pass. --- CHANGELOG.md | 1 + src/easylimit/rate_limiter.py | 22 +++++++++++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dd6efb..cc9039a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed call tracking bug where `try_acquire()` and `async_try_acquire()` did not increment `call_count` when `track_calls=True` - Fixed call tracking bug where `acquire()` did not increment `call_count` when called directly (outside context manager) with `track_calls=True` +- Fixed inefficient double-locking in `_record_call()` implementation - All acquisition methods now consistently track calls when tracking is enabled ### Changed diff --git a/src/easylimit/rate_limiter.py b/src/easylimit/rate_limiter.py index fb3d791..3736ff1 100644 --- a/src/easylimit/rate_limiter.py +++ b/src/easylimit/rate_limiter.py @@ -420,15 +420,19 @@ def _try_acquire_sync(self) -> bool: return False def _record_call(self, delay: float) -> None: - """Record tracking info under sync lock.""" - with self.lock: - self._call_count += 1 - now_ts = time.time() - self._timestamps.append(now_ts) - self._delays.append(delay) - self._last_call_time = datetime.now() - cutoff_time = now_ts - self._history_window - self._timestamps = [ts for ts in self._timestamps if ts >= cutoff_time] + """ + Record tracking info (caller must hold self.lock). + + Args: + delay: Time spent waiting for token acquisition + """ + self._call_count += 1 + now_ts = time.time() + self._timestamps.append(now_ts) + self._delays.append(delay) + self._last_call_time = datetime.now() + cutoff_time = now_ts - self._history_window + self._timestamps = [ts for ts in self._timestamps if ts >= cutoff_time] async def async_acquire(self, timeout: Optional[float] = None) -> bool: """