fix(azure-storage): close ContainerClient in sync lazy_load - #832
Merged
anjaliratnam-msft merged 3 commits intoJul 22, 2026
Merged
Conversation
`lazy_load` created a `ContainerClient` but never closed it, leaking the underlying HTTP transport / connection pool for every load. The async counterpart `alazy_load` already scopes its client with `async with`; mirror that on the sync path by using the client as a context manager so it is closed once iteration completes.
anjaliratnam-msft
left a comment
anjaliratnam-msft
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for bringing up this issue! I think this looks good, I just had one small comment.
Comment on lines
+212
to
+223
| def test_lazy_load_closes_container_client( | ||
| create_azure_blob_storage_loader: Callable[..., AzureBlobStorageLoader], | ||
| mock_container_client: Tuple[MagicMock, MagicMock], | ||
| ) -> None: | ||
| """The sync container client must be closed once iteration completes, | ||
| mirroring the ``async with`` used in ``alazy_load``.""" | ||
| _, mock_client = mock_container_client | ||
| loader = create_azure_blob_storage_loader() | ||
| list(loader.lazy_load()) | ||
| mock_client.__exit__.assert_called_once() | ||
|
|
||
|
|
Collaborator
There was a problem hiding this comment.
Instead of adding a new test for this, I think we can just add that assertion in test_lazy_load to help minimize the code changes.
…lose-container-client
Address review feedback: fold the container-client close assertion into `test_lazy_load` instead of adding a separate test, keeping the change minimal.
Contributor
Author
|
Makes sense — folded the assertion into The only other test change left is the two sync fixtures setting |
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
2 participants
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.
Description
AzureBlobStorageLoader.lazy_load(sync) constructs aContainerClientbut never closes it:The
ContainerClientowns an HTTP transport / connection pool, so every call tolazy_load(i.e. everyload()) leaks those connections until GC. The async counterpart already scopes its client correctly withasync with async_container_client:— only the sync path is missing the cleanup.Fix
Use the client as a context manager so it is closed once iteration finishes, mirroring
alazy_load:ContainerClientsupports the context-manager protocol and returns itself from__enter__, so behavior is unchanged aside from the client now being closed.Testing
Added
test_lazy_load_closes_container_client, which asserts the client is exited after a fulllazy_load. The two sync container-client mock fixtures now set__enter__.return_value = mock_clientso the mock matches the real client (which returnsselffrom__enter__); all existing sync load tests continue to exercise the same object. Verified the new test fails without the fix and passes with it.make lint(ruff + ruff format + mypy over package and tests) is clean; full unit suite passes.