diff options
author | Peter Wu <peter@lekensteyn.nl> | 2023-01-13 21:41:01 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-01-13 15:41:01 -0500 |
commit | 34e6829dd40e99e9ba47ea02fcfabda09e08d36e (patch) | |
tree | aa5e99bccf2d6d4d6d53efe457282984f1a8ca2d | |
parent | 22718ba59a193263bed8c52cc1abd5ee52358440 (diff) | |
download | docker-py-34e6829dd40e99e9ba47ea02fcfabda09e08d36e.tar.gz |
exec: fix file handle leak with container.exec_* APIs (#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 https://github.com/docker/docker-py/issues/1293
(Regression from https://github.com/docker/docker-py/pull/1130)
Signed-off-by: Peter Wu <pwu@cloudflare.com>
Co-authored-by: Milas Bowman <milas.bowman@docker.com>
-rw-r--r-- | docker/api/client.py | 11 | ||||
-rw-r--r-- | docker/api/exec_api.py | 18 |
2 files changed, 23 insertions, 6 deletions
diff --git a/docker/api/client.py b/docker/api/client.py index 7733d33..65b9d9d 100644 --- a/docker/api/client.py +++ b/docker/api/client.py @@ -406,6 +406,10 @@ class APIClient( yield from response.iter_content(chunk_size, decode) def _read_from_socket(self, response, stream, tty=True, demux=False): + """Consume all data from the socket, close the response and return the + data. If stream=True, then a generator is returned instead and the + caller is responsible for closing the response. + """ socket = self._get_raw_response_socket(response) gen = frames_iter(socket, tty) @@ -420,8 +424,11 @@ class APIClient( if stream: return gen else: - # Wait for all the frames, concatenate them, and return the result - return consume_socket_output(gen, demux=demux) + try: + # Wait for all frames, concatenate them, and return the result + return consume_socket_output(gen, demux=demux) + finally: + response.close() def _disable_socket_timeout(self, socket): """ Depending on the combination of python version and whether we're diff --git a/docker/api/exec_api.py b/docker/api/exec_api.py index 496308a..63df9e6 100644 --- a/docker/api/exec_api.py +++ b/docker/api/exec_api.py @@ -1,5 +1,6 @@ from .. import errors from .. import utils +from ..types import CancellableStream class ExecApiMixin: @@ -125,9 +126,10 @@ class ExecApiMixin: detach (bool): If true, detach from the exec command. Default: False tty (bool): Allocate a pseudo-TTY. Default: False - stream (bool): Stream response data. Default: False + stream (bool): Return response data progressively as an iterator + of strings, rather than a single string. socket (bool): Return the connection socket to allow custom - read/write operations. + read/write operations. Must be closed by the caller when done. demux (bool): Return stdout and stderr separately Returns: @@ -161,7 +163,15 @@ class ExecApiMixin: stream=True ) if detach: - return self._result(res) + try: + return self._result(res) + finally: + res.close() if socket: return self._get_raw_response_socket(res) - return self._read_from_socket(res, stream, tty=tty, demux=demux) + + output = self._read_from_socket(res, stream, tty=tty, demux=demux) + if stream: + return CancellableStream(output, res) + else: + return output |