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>

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