[Service Bus] aio AutoLockRenewer: prune completed futures to fix memory leak - #48403
Draft
EldertGrootenboer wants to merge 2 commits into
Draft
[Service Bus] aio AutoLockRenewer: prune completed futures to fix memory leak#48403EldertGrootenboer wants to merge 2 commits into
EldertGrootenboer wants to merge 2 commits into
Conversation
The async AutoLockRenewer appended each renewal future to an internal list and only drained it on close(), so a long-lived renewer (the documented one-per-app pattern) accumulated one future per registered message for its whole lifetime - memory growth proportional to the total messages processed, reclaimed only on close(). Store the futures in a set and remove each one on completion via renew_future.add_done_callback(self._futures.discard), keeping the collection bounded by the number of active renewals. close() is unchanged. Adds unit tests covering bounded-after-settle, close-awaits- active, errored-renewal-pruned, and the over-prune guard.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The implementation safely bounds retained futures and includes focused coverage for completion and shutdown behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes #48366 by pruning completed async lock-renewal futures, preventing unbounded memory growth.
Changes:
- Tracks active futures in a set and removes them on completion.
- Adds offline regression tests for completion, errors, and shutdown.
- Documents the fix in the changelog.
File summaries
| File | Description |
|---|---|
_async_auto_lock_renewer.py |
Prunes completed renewal futures. |
test_auto_lock_renewer_futures.py |
Adds regression coverage. |
CHANGELOG.md |
Records the memory-leak fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #48366.
The async
AutoLockRenewerappended each renewalasyncio.Futureto an internal list_futuresinregister()and never removed it on completion — the list was drained only byclose(). A long-lived renewer (the documented one-renewer-per-app pattern) therefore accumulated one future per registered message for its whole lifetime: memory grew proportional to the total messages processed, reclaimed only onclose().Fix
_futuresis now aset, andregister()registersrenew_future.add_done_callback(self._futures.discard)so each future is removed the moment its renewal coroutine completes (success, timeout, or error). The collection stays bounded by the number of active renewals rather than the all-time count ofregister()calls.close()is unchanged (await asyncio.wait(self._futures)still awaits any remaining active futures;asyncio.waitsnapshots its input, so callback-driven removal during the await is safe).typingimport.Tests
New
tests/unittests/test_auto_lock_renewer_futures.py(offline, no network):close()still awaits an active renewalThe full offline unit suite passes. The Python sync renewer is unaffected (it self-drains via a
queue.Queue), and the .NET/Java/JS SDKs prune renewal handles on completion, so this is an aio-only fix.