docker-py
docker-py copied to clipboard
docker
exec: fix file handle leak with container.exec_* APIs
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 https://github.com/docker/docker-py/issues/1293 (Regression from https://github.com/docker/docker-py/pull/1130)
Signed-off-by: Peter Wu [email protected]
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, https://github.com/pytest-dev/pytest/issues/4143 or manually changing sys.stderr.)
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!
