Add context manager to the client API #2815
Conversation
`ApiClient` inherence from `requests.session` that implements `__enter__` and `__exit__` methods and the issue docker#2808 asks to add the same behaviour to the `DockerClient`. At the same time, issue docker#2766 reports a file descriptor leak when `_get_raw_response_socket` is called. To fix both requests, I added a `set` to the `ApiClient` instance to track every socket open by `_get_raw_response_socket` and close when the `close` method is called. I added a test, and documentation now brings the context manager as an example. Signed-off-by: Felipe Ruhland <felipe.ruhland@gmail.com>
| @@ -209,6 +218,20 @@ def __init__(self, base_url=None, version=None, | |||
| 'library.'.format(MINIMUM_DOCKER_API_VERSION) | |||
| ) | |||
|
|
|||
| def __exit__(self, exc_type, exc_value, traceback): | |||
| super().__exit__(exc_type, exc_value, traceback) | |||
There was a problem hiding this comment.
We're losing any potential return value here, hence overriding any attempt by the super class to suppress an in-flight exception.
| @@ -209,6 +218,20 @@ def __init__(self, base_url=None, version=None, | |||
| 'library.'.format(MINIMUM_DOCKER_API_VERSION) | |||
| ) | |||
|
|
|||
| def __exit__(self, exc_type, exc_value, traceback): | |||
| super().__exit__(exc_type, exc_value, traceback) | |||
| self._close_sockets() | |||
There was a problem hiding this comment.
I'm somewhat unclear on the flow here. If we don't define __exit__, won't the parent's __exit__ call our close method anyway, so the close definition later would be sufficient.
I might be overlooking something though.
There was a problem hiding this comment.
I also feel like in the current flow, we'll super-call close twice, once in super().__exit__, and then again in our own close called from here. That seems odd, and I see a double-close in one of the tests too. Is there some other issue with close that we're covering up by recalling close() so much?
| url = client._url('/no-tty') | ||
| resp = client._post(url, stream=True) | ||
| return client._read_from_socket( | ||
| resp, stream=stream, tty=tty, demux=demux) |
There was a problem hiding this comment.
Is this deliberately introducing a leak by lifting the APIClient out of a with?
Would it be better to make this a generator instead or something?
|
This should also close #2457. |
ApiClientinherence fromrequests.sessionthat implements__enter__and__exit__methods and the issue #2808 asks to add thesame behaviour to the
DockerClient.At the same time, issue #2766 reports a file descriptor leak when
_get_raw_response_socketis called.To fix both requests, I added a
setto theApiClientinstance totrack every socket open by
_get_raw_response_socketand close whenthe
closemethod is called.I added a test, and documentation now brings the context manager as an
example.
Signed-off-by: Felipe Ruhland felipe.ruhland@gmail.com