Release HTTP connection back to the pool after making an API request

[jdufresne]

Fixes warnings in application code:

  ResourceWarning: unclosed <ssl.SSLSocket fd=X, family=AddressFamily.AF_INET, type=2049, proto=6, laddr=('...', ...), raddr=('52.95.145.10', 443)>

The warning normally only occurs for 4XX responses.

Fixes boto/boto3#454

[codecov-io]

Codecov Report

Merging #1231 into develop will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #1231      +/-   ##
===========================================
+ Coverage    98.03%   98.03%   +<.01%     
===========================================
  Files           45       45              
  Lines         7342     7344       +2     
===========================================
+ Hits          7198     7200       +2     
  Misses         144      144

Impacted Files	Coverage Δ
botocore/endpoint.py	97.24% <100%> (+0.03%)	⬆️

---

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 46ac74a...ecd9213. Read the comment docs.

[dstufft]

This is interesting, because this technically changes the semantics of the API to return a request object that no longer has an open connection instead of one that does. This object eventually gets passed into the event system which external parties can register events that do things.

That being said, I think that at this point closing the http connection should be fine, because I don't think there's anything left that can be done (unless someone is doing something crazy like pulling the connection off of the already consumed request and re-using it externally).

I'm inclined to accept this PR (with the changes I described in another comment), but let me double check on our API compatibility with this.

[dstufft]

I think this would be better written as:

with http_response:
    response_dict = ...
    parser = ...
    parsed_response = ...
    return (http_response, parsed_response), None

It has the same effect, but it ensures it gets closed even if there is a exception here.

[jamesls]

Is this safe? I don't think this accounts for streaming responses does it?

[dstufft]

Talking offline, this is probably going to require a lot more work to handle streaming responses. We pass the http_response as part of the object we return in some cases so we can't close it until after the user has consumed the response.

[jdufresne]

I've modified the code to use a context manager.

What do you think about this alternative implementation which moves the closing to BaseClient._make_api_call() so as to delay calling close until after the 'after-call.{endpoint_prefix}.{operation_name}' event is emitted.

diff --git a/botocore/client.py b/botocore/client.py
index e7a81c5..5ec6974 100644
--- a/botocore/client.py
+++ b/botocore/client.py
@@ -10,6 +10,7 @@
 # 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.
+import contextlib
 import logging
 import functools
 
@@ -585,20 +586,21 @@ class BaseClient(object):
             http, parsed_response = self._endpoint.make_request(
                 operation_model, request_dict)
 
-        self.meta.events.emit(
-            'after-call.{endpoint_prefix}.{operation_name}'.format(
-                endpoint_prefix=self._service_model.endpoint_prefix,
-                operation_name=operation_name),
-            http_response=http, parsed=parsed_response,
-            model=operation_model, context=request_context
-        )
+        with contextlib.closing(http):
+            self.meta.events.emit(
+                'after-call.{endpoint_prefix}.{operation_name}'.format(
+                    endpoint_prefix=self._service_model.endpoint_prefix,
+                    operation_name=operation_name),
+                http_response=http, parsed=parsed_response,
+                model=operation_model, context=request_context
+            )
 
-        if http.status_code >= 300:
-            error_code = parsed_response.get("Error", {}).get("Code")
-            error_class = self.exceptions.from_code(error_code)
-            raise error_class(parsed_response, operation_name)
-        else:
-            return parsed_response
+            if http.status_code >= 300:
+                error_code = parsed_response.get("Error", {}).get("Code")
+                error_class = self.exceptions.from_code(error_code)
+                raise error_class(parsed_response, operation_name)
+            else:
+                return parsed_response
 
     def _convert_to_request_dict(self, api_params, operation_model,
                                  context=None):

[jdufresne]

I'm motivated to fix this runtime warning as it causes a lot of noise in my logs. If the original approach isn't workable, I'm very willing to investigate other solution or incorporate necessary feedback.

[kdaily]

The solution presented here as a fix for boto/boto3#454 was discussed recently as to why we would not be able to accept this change:

boto/boto3#454 (comment)

Closing this out, please check on boto/boto3#454 for updates on proposed solutions. Thanks!