Repository: boto/botocore
Pull request #1810 (2019-08-16 10:40:16)

Expose close() method in client instances
Although the (now outdated) #1499 (add `__del__`) and #1231 (close/release connection back to pool) try to address https://github.com/boto/boto3/issues/454 , this PR suggests simply exposing a `close` method on `botocore.client` client instances. It should then be possible to e.g. use `contextlib.closing` or to manually close the client after usage:

```python
with closing(session.client(service_name='secretsmanager', ...)) as client:
    secret = client.get_secret_value('my_secret')
    # Make any additional calls for the duration of the client instance
```

~I have not added any tests since I wasn't sure what would be appropriate. This additional method however should not affect any existing behaviour.~ Have added test to exercise the new methods.

codecov-io: # [Codecov](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=h1) Report
> Merging [#1810](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=desc) into [develop](https://codecov.io/gh/boto/botocore/commit/2b307062843e6a5726da9470c062cfbdb50a3e0e?src=pr&el=desc) will **increase** coverage by `0.29%`.
> The diff coverage is `37.5%`.

[![Impacted file tree graph](https://codecov.io/gh/boto/botocore/pull/1810/graphs/tree.svg?width=650&token=QOLXw6XSza&height=150&src=pr)](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           develop   #1810      +/-   ##
==========================================
+ Coverage     92.2%   92.5%   +0.29%     
==========================================
  Files           53      53              
  Lines         9958    9966       +8     
==========================================
+ Hits          9182    9219      +37     
+ Misses         776     747      -29
```


| [Impacted Files](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [botocore/httpsession.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvaHR0cHNlc3Npb24ucHk=) | `90.9% <25%> (-1.54%)` | :arrow_down: |
| [botocore/endpoint.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvZW5kcG9pbnQucHk=) | `97.9% <50%> (-0.68%)` | :arrow_down: |
| [botocore/client.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvY2xpZW50LnB5) | `99.52% <50%> (-0.24%)` | :arrow_down: |
| [botocore/utils.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvdXRpbHMucHk=) | `97.98% <0%> (+0.16%)` | :arrow_up: |
| [botocore/credentials.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvY3JlZGVudGlhbHMucHk=) | `98.85% <0%> (+0.34%)` | :arrow_up: |
| [botocore/hooks.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvaG9va3MucHk=) | `96.38% <0%> (+0.4%)` | :arrow_up: |
| [botocore/handlers.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvaGFuZGxlcnMucHk=) | `96.92% <0%> (+0.47%)` | :arrow_up: |
| [botocore/compat.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvY29tcGF0LnB5) | `93.92% <0%> (+4.97%)` | :arrow_up: |
| [botocore/awsrequest.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvYXdzcmVxdWVzdC5weQ==) | `98.36% <0%> (+5.9%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=footer). Last update [2b30706...1d118f9](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

github-actions[bot]: Greetings! It looks like this issue hasn’t been active in longer than one year. We encourage you to check if this is still an issue in the latest release. Because it has been longer than one year since the last update on this, and in the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment to prevent automatic closure, or if the issue is already closed, please feel free to reopen it.
codecov-commenter: # [Codecov](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=h1) Report
> Merging [#1810](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=desc) into [develop](https://codecov.io/gh/boto/botocore/commit/8c842bbcd55ebe00447bc29e7f1daa22199904fa&el=desc) will **decrease** coverage by `0.59%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/boto/botocore/pull/1810/graphs/tree.svg?width=650&height=150&src=pr&token=QOLXw6XSza)](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           develop    #1810      +/-   ##
===========================================
- Coverage    93.11%   92.52%   -0.60%     
===========================================
  Files           60       53       -7     
  Lines        11114     9966    -1148     
===========================================
- Hits         10349     9221    -1128     
+ Misses         765      745      -20     
```


| [Impacted Files](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [botocore/client.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvY2xpZW50LnB5) | `99.76% <100.00%> (+1.17%)` | :arrow_up: |
| [botocore/endpoint.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvZW5kcG9pbnQucHk=) | `98.60% <100.00%> (+0.56%)` | :arrow_up: |
| [botocore/httpsession.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvaHR0cHNlc3Npb24ucHk=) | `92.61% <100.00%> (+0.12%)` | :arrow_up: |
| [botocore/compat.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvY29tcGF0LnB5) | `93.92% <0.00%> (-1.46%)` | :arrow_down: |
| [botocore/docs/bcdoc/docstringparser.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvZG9jcy9iY2RvYy9kb2NzdHJpbmdwYXJzZXIucHk=) | `99.15% <0.00%> (-0.85%)` | :arrow_down: |
| [botocore/model.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvbW9kZWwucHk=) | `97.82% <0.00%> (-0.39%)` | :arrow_down: |
| [botocore/utils.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvdXRpbHMucHk=) | `97.98% <0.00%> (-0.34%)` | :arrow_down: |
| [botocore/docs/bcdoc/restdoc.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvZG9jcy9iY2RvYy9yZXN0ZG9jLnB5) | `96.26% <0.00%> (-0.17%)` | :arrow_down: |
| [botocore/credentials.py](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree#diff-Ym90b2NvcmUvY3JlZGVudGlhbHMucHk=) | `98.50% <0.00%> (-0.14%)` | :arrow_down: |
| ... and [32 more](https://codecov.io/gh/boto/botocore/pull/1810/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=footer). Last update [8c842bb...f2ee289](https://codecov.io/gh/boto/botocore/pull/1810?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

vtermanis: > We encourage you to check if this is still an issue in the latest release

There still is no `close()` (or equivalent method).
vtermanis: Rebased to be against current top of `develop` branch again.
akx: 👍 , would love to see this get merged – I currently have to rely on 

```
def close_client(client: BaseClient):
    # See https://github.com/boto/botocore/pull/1810 ...
    try:
        client._endpoint.http_session._manager.clear()
        return True
    except Exception:
        return False
```
akx: Could we get e.g. @nateprewitt to take a look at this?
hannes-ucsc: If I wanted the client to be long-lived in order to maximize the benefit of the connection pool, would I still get the warnings from #454 even with the changes from this PR?
nateprewitt: Hi @vtermanis, I left a response to https://github.com/boto/boto3/issues/454 a bit earlier. I think we're going to try and get this added to an upcoming release. I've rebased your change onto our current `develop` branch to get CI running again.

I think we'll likely want to add some doc changes and potentially a couple more tests before merging. I'm still working through what those will be and will update once we have more. Please let me know if you'd like to continue this work or would rather I build on top of your existing PR. Thanks!
nateprewitt: > If I wanted the client to be long-lived in order to maximize the benefit of the connection pool, would I still get the warnings from https://github.com/boto/botocore/issues/454 even with the changes from this PR?

@hannes-ucsc, to answer your question, you would likely still see these warnings for long-lived clients. The existence of the ConnectionPool is what's causing these warnings to arise. The outcome of this PR will only provide the ability to forcibly cleanup client ConnectionPools when you're finished with them.
vtermanis: @nateprewitt - Thanks for considering this. It's been a while since I look at the codebase (and I don't have quite as much time as I used to - though I guess who has, eh?)  - so I think you building on top of this will be more efficient at this point.

Patch:
diff --git a/botocore/client.py b/botocore/client.py
index bbcb35a8c..d54c47eac 100644
--- a/botocore/client.py
+++ b/botocore/client.py
@@ -839,6 +839,10 @@ class BaseClient:
             f"'{self.__class__.__name__}' object has no attribute '{item}'"
         )
 
+    def close(self):
+        """Closes underlying endpoint connections."""
+        self._endpoint.close()
+
     def _register_handlers(self):
         # Register the handler required to sign requests.
         service_id = self.meta.service_model.service_id.hyphenize()
diff --git a/botocore/endpoint.py b/botocore/endpoint.py
index 7814e5ac9..adc622c25 100644
--- a/botocore/endpoint.py
+++ b/botocore/endpoint.py
@@ -107,6 +107,9 @@ class Endpoint:
     def __repr__(self):
         return f'{self._endpoint_prefix}({self.host})'
 
+    def close(self):
+        self.http_session.close()
+
     def make_request(self, operation_model, request_dict):
         logger.debug(
             "Making request for %s with params: %s",
diff --git a/botocore/httpsession.py b/botocore/httpsession.py
index 300d09e4e..29b210377 100644
--- a/botocore/httpsession.py
+++ b/botocore/httpsession.py
@@ -423,6 +423,11 @@ class URLLib3Session:
         transfer_encoding = ensure_bytes(transfer_encoding)
         return transfer_encoding.lower() == b'chunked'
 
+    def close(self):
+        self._manager.clear()
+        for manager in self._proxy_managers.values():
+            manager.clear()
+
     def send(self, request):
         try:
             proxy_url = self._proxy_config.proxy_url_for(request.url)


Test patch:
diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py
index 2566816f2..e952ddd0b 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -10,6 +10,8 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
 # ANY KIND, either express or implied. See the License for the specific
 # language governing permissions and limitations under the License.
+from contextlib import closing
+
 import botocore
 import botocore.config
 from botocore import client, exceptions, hooks
@@ -1585,6 +1587,26 @@ class TestAutoGeneratedClient(unittest.TestCase):
         request_dict = self.endpoint.make_request.call_args[0][1]
         self.assertEqual(request_dict['url'], expected_url)
 
+    def test_client_close(self):
+        creator = self.create_client_creator()
+        service_client = creator.create_client(
+            'myservice', 'us-west-2', credentials=self.credentials
+        )
+
+        service_client.close()
+        self.endpoint.close.assert_called_once_with()
+
+    def test_client_close_context_manager(self):
+        creator = self.create_client_creator()
+        with closing(
+            creator.create_client(
+                'myservice', 'us-west-2', credentials=self.credentials
+            )
+        ) as service_client:
+            service_client.test_operation(Foo='baz')
+
+        self.endpoint.close.assert_called_once_with()
+
 
 class TestClientErrors(TestAutoGeneratedClient):
     def add_error_response(self, error_response):
diff --git a/tests/unit/test_endpoint.py b/tests/unit/test_endpoint.py
index 946d16bec..9fe6e9f4d 100644
--- a/tests/unit/test_endpoint.py
+++ b/tests/unit/test_endpoint.py
@@ -201,6 +201,10 @@ class TestEndpointFeatures(TestEndpointBase):
         }
         self.assertEqual(response, expected_response)
 
+    def test_close(self):
+        self.endpoint.close()
+        self.endpoint.http_session.close.assert_called_once_with()
+
 
 class TestRetryInterface(TestEndpointBase):
     def setUp(self):
diff --git a/tests/unit/test_http_session.py b/tests/unit/test_http_session.py
index 4f7605aa6..bd591e110 100644
--- a/tests/unit/test_http_session.py
+++ b/tests/unit/test_http_session.py
@@ -496,3 +496,23 @@ class TestURLLib3Session(unittest.TestCase):
 
         session.send(self.request.prepare())
         self.assert_request_sent(chunked=False)
+
+    def test_close(self):
+        session = URLLib3Session()
+        session.close()
+        self.pool_manager.clear.assert_called_once_with()
+
+    def test_close_proxied(self):
+        proxies = {'https': 'http://proxy.com', 'http': 'http://proxy2.com'}
+        session = URLLib3Session(proxies=proxies)
+        for proxy, proxy_url in proxies.items():
+            self.request.url = '%s://example.com/' % proxy
+            session.send(self.request.prepare())
+
+        session.close()
+        self.proxy_manager_fun.return_value.clear.assert_called_with()
+        # One call for pool manager, one call for each of the proxies
+        self.assertEqual(
+            self.proxy_manager_fun.return_value.clear.call_count,
+            1 + len(proxies),
+        )