Repository navigation
fix: free the retrying object when a call returns - #758
spicysharkco wants to merge 3 commits into
Conversation
Every call of a @retry-decorated function runs on a copy of the retrying object, and BaseRetrying.iter() left that copy's iter_state.actions populated with bound methods of the copy. The copy then referenced itself through its own thread-local state, so only the cycle collector could free it: 11 to 14 unreachable objects per sync call and 19 to 22 per async call (about 1.6 KB and 2.3 KB), even when the first attempt succeeded. Clear the action list once iter() is done with it, also when an action raises. The list is only read inside iter() and is rebuilt on the next call; statistics, retry_state and the outcome are not touched. Retrying, AsyncRetrying and TornadoRetrying are all fixed through iter(). A call that gives up with a failed last attempt still forms the usual exception/traceback cycle through the frame that caught the exception. That case is reduced, not eliminated: 50 to 45 unreachable objects per sync call and 75 to 58 per async call on 3.10 (53 to 48 and 78 to 61 on 3.14). "async for attempt in AsyncRetrying()" also still leaves a cycle, because the retry state points back at the controller. It drops from 35 to 24 objects on 3.10 (38 to 27 on 3.14).
|
Tick the box to add this pull request to the merge queue (same as
|
|
One extra terminal path might be worth keeping in the tests: I added a small test-only commit that checks the controller's weak reference directly, without relying on a whole-process garbage count. The fallback and attempt statistics are checked before dropping the last strong reference. It restores the collector's previous setting and skips non-CPython implementations. On CPython 3.14.4, the test passes at After taking the commit, the focused command is: python -m pytest tests/test_fallback_controller_lifetime.py -qI didn't run the full suite, docs/type checks, or pinned Ruff version; focused lint/format checks used Ruff 0.15.11. Feel free to use the test if it helps. |
Add a case to the reference-cycle tests, sync and async, where every attempt is retried and retry_error_callback returns a fallback value. The call must return the fallback, report the attempt statistics, and leave no garbage for the cycle collector. Co-authored-by: carlitogwapito <20962552+carlitogwapito@users.noreply.github.com>
|
Thanks @carlitogwapito, good catch: the fallback path wasn't covered here. I added it to both stacks in 792d750, with you as co-author. It uses the classes' existing garbage-count check instead of a weakref, so the reference-cycle tests stay consistent. Both fail on 32ac68f and pass on this branch. |
iter() binds the action list to a local before running it and clears that list, not whatever iter_state.actions points to once the actions have run. Add a test per class where a before callback replaces the list mid-iteration, as another call on a shared controller would: the iterated list must end up empty and the replacement must be left alone. Clearing self.iter_state.actions instead fails both tests.
|
This is waiting for a review. It merges cleanly, CI passes, and the retry_error_callback fallback case raised above is covered in 792d750. |
Every call of a
@retry-decorated function leaves a reference cycle behind, even when the first attempt succeeds.The decorator runs each call on a fresh
copy()of the retrying object.BaseRetrying.iter()collects the actions of an attempt initer_state.actions, and those actions are bound methods of the retrying object (self._run_retry,self._post_retry_check_actions, ...). The list is never emptied, so the copy refers to itself through its own thread-local state and only the cycle collector can free it. Arguments and return values are not kept alive by the cycle (a call with a 4 MB argument and a 4 MB result still holds ~1.7 KB), but the retrying object, its thread-local storage, the iteration state, and the bound methods and closures in the action list pile up until the next collection.Unreachable objects per call, with
gcdisabled and onegc.collect()after 100 calls (CPython 3.10 / 3.14):Retrying()(fn)for attempt in Retrying()That is about 1.6 KB per sync call and 2.3 to 2.7 KB per async call held until the next collection, now under 20 bytes.
The fix clears the action list when
iter()is done with it, in afinallyso it also runs when an action raises (for example theRetryErrorthat gives up). The list is only read insideiter()and is rebuilt by the next_begin_iter();statistics,retry_stateand the outcome are not touched. It clears the list that thisiter()call iterated, not whateveriter_state.actionspoints to once the actions have run.Retrying,AsyncRetryingandTornadoRetryingall go throughiter(), so the change is the same few lines intenacity/__init__.pyandtenacity/asyncio/__init__.py.A call that gives up with a failed last attempt still leaves the usual cycle between the exception, its traceback and the frame that caught it. This change reduces that case but does not eliminate it. Unreachable objects per call, before and after (CPython 3.10 / 3.14):
RetryErrorreraise=TrueRetryErrorreraise=Trueasync for attempt in AsyncRetrying()also still leaves a cycle: the controller keeps itsRetryCallStatein_retry_state, and the state'sretry_objectpoints back at the controller. The loop drops from 35 to 24 unreachable objects on 3.10 and from 38 to 27 on 3.14, not to zero. This PR covers the decorator, direct calls and the syncfor attemptloop.Open PR #757 changes the same lines of
AsyncRetrying.iter(). Whichever of the two lands second needs a small rebase. If #757 lands first, the async hunk here becomes unnecessary, while the sync change, the tests and the release note still apply.A decorated call that does not retry is not slower: minimum of 15 runs, collector enabled, sync 14.2 to 14.5 us before and 13.3 to 13.6 us after on 3.10, 8.7 to 8.8 us before and 7.8 to 7.9 us after on 3.14; async is within noise.
Tests:
TestReferenceCycles(tests/test_tenacity.py) andTestAsyncReferenceCycles(tests/test_asyncio.py) run a decorated function, a controller called directly, and (sync) an attempt loop withgcdisabled, and assert thatgc.collect()finds nothing. They cover first-attempt success, retry-then-success, and a call where every attempt is retried andretry_error_callbackreturns a fallback, with and withoutreraise, and check thatstatisticsstill report the attempts. One test in each class uses astopcallable that raises, to cover thefinally; it fails if thetry/finallyis removed and theclear()is kept after the loop. Another replacesiter_state.actionsfrom abeforecallback, as a second call on a shared controller would, and checks that the iterated list is cleared and the replacement is left alone; it fails if thefinallyclearsself.iter_state.actionsinstead.tests/test_tornado.pyhas a matching test forTornadoRetrying. All 18 fail on main and pass with the change. The suite passes on 3.10 (202 passed, 1 skipped) and 3.14 (203 passed, also with-W error), and ruff, strict mypy,reno lintand the Sphinx doctest and HTML builds pass. A release note is included.