Expose close() method in client instances

[vtermanis]

Although the (now outdated) #1499 (add __del__) and #1231 (close/release connection back to pool) try to address boto/boto3#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:

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 Report

Merging #1810 into develop will increase coverage by 0.29%.
The diff coverage is 37.5%.

@@            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	Coverage Δ
botocore/httpsession.py	90.9% <25%> (-1.54%)	⬇️
botocore/endpoint.py	97.9% <50%> (-0.68%)	⬇️
botocore/client.py	99.52% <50%> (-0.24%)	⬇️
botocore/utils.py	97.98% <0%> (+0.16%)	⬆️
botocore/credentials.py	98.85% <0%> (+0.34%)	⬆️
botocore/hooks.py	96.38% <0%> (+0.4%)	⬆️
botocore/handlers.py	96.92% <0%> (+0.47%)	⬆️
botocore/compat.py	93.92% <0%> (+4.97%)	⬆️
botocore/awsrequest.py	98.36% <0%> (+5.9%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b30706...6387309. Read the comment docs.

[github-actions]

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 Report

Merging #1810 (1495b2c) into develop (df7662b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1810   +/-   ##
========================================
  Coverage    95.29%   95.30%           
========================================
  Files           60       60           
  Lines        12252    12260    +8     
========================================
+ Hits         11676    11684    +8     
  Misses         576      576           

Impacted Files	Coverage Δ
botocore/client.py	98.72% <100.00%> (+<0.01%)	⬆️
botocore/endpoint.py	97.81% <100.00%> (+0.02%)	⬆️
botocore/httpsession.py	91.73% <100.00%> (+0.13%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df7662b...1495b2c. Read the comment docs.

[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 boto/boto3#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 #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.

If I need to sign/tick some contribution-related box somewhere, let me know. (Though if this is not needed - that's fine by me.)

[nateprewitt]

Alright, things are rebased and the tests are cleaned up. Let's ship it! Thanks again, @vtermanis

[sparrowt]

boto/boto3#454 (comment) says the following regarding this PR:

#1810 has some surprising side effects that are going to catch people. When the user calls close, it invokes it on the PoolManager. That not only closes the connections for the client/resource in use, but ALL clients and resources created by the underlying Session. This may lead to unexpected failures in parallelized environments and introduces a new level of management required for client cleanup. You MUST ensure all users of the Session are ready for teardown before invoking close.
...
We do need to make sure the implications are clearly documented for users though.

Is that still the case with the final code that was merged? (from an outsider's brief look it would appear so) I couldn't find any documentation either in this PR or generally on boto3/botocore docs so I'm wondering whether close() is safe to use or of the surprising side effects are still a risk.

[mmerickel]

If that’s the case shouldn’t the close() method be on the session and not the client/resource? It’s where I looked for a method first before finding this ticket and using the version in the client as in the comments above.