Skip to content

exec: fix file handle leak with container.exec_* APIs #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 4, 2022

Conversation

Nitrooo
Copy link
Owner

@Nitrooo Nitrooo commented Jul 4, 2022

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

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>
@Nitrooo Nitrooo merged commit cffe745 into Nitrooo:master Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResourceWarning: unclosed <socket.socket>
2 participants