exec: fix file handle leak with container.exec_* APIs#2320
Merged
milas merged 2 commits intodocker:mainfrom Jan 13, 2023
Merged
exec: fix file handle leak with container.exec_* APIs#2320milas merged 2 commits intodocker:mainfrom
milas merged 2 commits intodocker:mainfrom
Conversation
Requests with stream=True MUST be closed or else the connection will
never be returned to the connection pool. Both ContainerApiMixin.attach
and ExecApiMixin.exec_start were leaking in the stream=False case.
exec_start was modified to follow attach for the stream=True case as
that allows the caller to close the stream when done (untested).
Tested with:
# Test exec_run (stream=False) - observe one less leak
make integration-test-py3 file=models_containers_test.py' -k test_exec_run_success -vs -W error::ResourceWarning'
# Test exec_start (stream=True, fully reads from CancellableStream)
make integration-test-py3 file=api_exec_test.py' -k test_execute_command -vs -W error::ResourceWarning'
After this change, one resource leak is removed, the remaining resource
leaks occur because none of the tests call client.close().
Fixes docker#1293
(Regression from docker#1130)
Signed-off-by: Peter Wu <pwu@cloudflare.com>
c7bc674 to
c9072fc
Compare
|
This fixes the problem I observed and I hope it gets incorporated soon! |
|
Are there any technical reasons why this hasn't been merged, or has it just slipped under the radar? Tentatively tagging @shin- in case they may be able to help! |
|
@milas can you please review and possibly merge this fix? Thank you. |
felixfontein
added a commit
to felixfontein/community.docker
that referenced
this pull request
Feb 11, 2023
…py#2320) Requests with stream=True MUST be closed or else the connection will never be returned to the connection pool. Both ContainerApiMixin.attach and ExecApiMixin.exec_start were leaking in the stream=False case. exec_start was modified to follow attach for the stream=True case as that allows the caller to close the stream when done (untested). Tested with: # Test exec_run (stream=False) - observe one less leak make integration-test-py3 file=models_containers_test.py' -k test_exec_run_success -vs -W error::ResourceWarning' # Test exec_start (stream=True, fully reads from CancellableStream) make integration-test-py3 file=api_exec_test.py' -k test_execute_command -vs -W error::ResourceWarning' After this change, one resource leak is removed, the remaining resource leaks occur because none of the tests call client.close(). Fixes docker/docker-py#1293 (Regression from docker/docker-py#1130) Cherry-picked from docker/docker-py@34e6829 Co-authored-by: Peter Wu <pwu@cloudflare.com> Co-authored-by: Milas Bowman <milas.bowman@docker.com>
felixfontein
added a commit
to ansible-collections/community.docker
that referenced
this pull request
Feb 12, 2023
…py#2320) (#582) Requests with stream=True MUST be closed or else the connection will never be returned to the connection pool. Both ContainerApiMixin.attach and ExecApiMixin.exec_start were leaking in the stream=False case. exec_start was modified to follow attach for the stream=True case as that allows the caller to close the stream when done (untested). Tested with: # Test exec_run (stream=False) - observe one less leak make integration-test-py3 file=models_containers_test.py' -k test_exec_run_success -vs -W error::ResourceWarning' # Test exec_start (stream=True, fully reads from CancellableStream) make integration-test-py3 file=api_exec_test.py' -k test_execute_command -vs -W error::ResourceWarning' After this change, one resource leak is removed, the remaining resource leaks occur because none of the tests call client.close(). Fixes docker/docker-py#1293 (Regression from docker/docker-py#1130) Cherry-picked from docker/docker-py@34e6829 Co-authored-by: Peter Wu <pwu@cloudflare.com> Co-authored-by: Milas Bowman <milas.bowman@docker.com>
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
5 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.
Requests with stream=True MUST be closed or else the connection will
never be returned to the connection pool. Both ContainerApiMixin.attach
and ExecApiMixin.exec_start were leaking in the stream=False case.
exec_start was modified to follow attach for the stream=True case as
that allows the caller to close the stream when done (untested).
Tested with:
After this change, one resource leak is removed, the remaining resource
leaks occur because none of the tests call client.close().
Fixes #1293
(Regression from #1130)
Signed-off-by: Peter Wu pwu@cloudflare.com
Relevant documentation:
https://2.python-requests.org//en/master/user/advanced/#body-content-workflow
Note: it is not easy to write a test to catch this issue. For one, lot of other tests also suffer from the resource leak issue (
client.close()not being called). The other issue is that this error is only written to stderr.While pytest supports the capsys fixture to capture stderr, this is not easily possible due to the use of unittest which does not allow fixtures as function parameters: https://docs.pytest.org/en/latest/unittest.html
(Potential workarounds include using request.getfixturevalue, pytest-dev/pytest#4143 or manually changing sys.stderr.)