reuse existing client

[guilhermef]

This PR is an attempt to reuse the same client across S3 requests, may fix #15

[coveralls]

Pull Request Test Coverage Report for Build 2223643706

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
• 7 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.3%) to 91.239%

---

Files with Coverage Reduction	New Missed Lines	%
thumbor_aws/storage.py	1	92.94%
thumbor_aws/s3_client.py	6	87.72%

Totals
Change from base Build 2197971738:	-0.3%
Covered Lines:	302
Relevant Lines:	331

---

💛 - Coveralls

[heynemann]

Does it matter that we restart the client? From what I understood from the docs the client should be re-created every time (it does not hold any connections, does it?).

[guilhermef]

Does it matter that we restart the client? From what I understood from the docs the client should be re-created every time (it does not hold any connections, does it?).

Based on this comment, it seems that the client can reuse the connections aio-libs/aiobotocore#928

[heynemann]

We should be calling __ aexit__ shouldn't we? I think the correct way here is to assing self.s3_client to client and have the rest of the codebase do:

with s3_client as client:
    # Whatever you need to do!

This way we ensure proper disposition of resources, right?

[guilhermef]

We might not need all the changes I've made; the session object should be enough to reuse the same HTTP connections.

I would need @oliverschewe help to validate this.

[guilhermef]

@heynemann I've updated the PR, to only reuse the S3Client object, but to still keep the with s3_client as client usage.

[heynemann]

LGTM with minor nits!

[heynemann]

Suggested change

	if not self._s3_client:
	self._s3_client = S3Client(context)
	self._s3_client.configuration = {
	"region_name": context.config.AWS_LOADER_REGION_NAME,
	"secret_access_key": context.config.AWS_LOADER_S3_SECRET_ACCESS_KEY,
	"access_key_id": context.config.AWS_LOADER_S3_ACCESS_KEY_ID,
	"endpoint_url": context.config.AWS_LOADER_S3_ENDPOINT_URL,
	"bucket_name": context.config.AWS_LOADER_BUCKET_NAME,
	"root_path": context.config.AWS_LOADER_ROOT_PATH,
	}
	if self._s3_client.compatibility_mode is True:
	self._s3_client.configuration[
	"region_name"
	] = context.config.TC_AWS_REGION
	self._s3_client.configuration[
	"endpoint_url"
	] = context.config.TC_AWS_ENDPOINT
	self._s3_client.configuration[
	"bucket_name"
	] = context.config.TC_AWS_LOADER_BUCKET
	self._s3_client.configuration[
	"root_path"
	] = context.config.TC_AWS_LOADER_ROOT_PATH
	return self._s3_client
	if self._s3_client is not None:
	return self._s3_client
	self._s3_client = S3Client(context)
	self._s3_client.configuration = {
	"region_name": context.config.AWS_LOADER_REGION_NAME,
	"secret_access_key": context.config.AWS_LOADER_S3_SECRET_ACCESS_KEY,
	"access_key_id": context.config.AWS_LOADER_S3_ACCESS_KEY_ID,
	"endpoint_url": context.config.AWS_LOADER_S3_ENDPOINT_URL,
	"bucket_name": context.config.AWS_LOADER_BUCKET_NAME,
	"root_path": context.config.AWS_LOADER_ROOT_PATH,
	}
	if self._s3_client.compatibility_mode is True:
	self._s3_client.configuration[
	"region_name"
	] = context.config.TC_AWS_REGION
	self._s3_client.configuration[
	"endpoint_url"
	] = context.config.TC_AWS_ENDPOINT
	self._s3_client.configuration[
	"bucket_name"
	] = context.config.TC_AWS_LOADER_BUCKET
	self._s3_client.configuration[
	"root_path"
	] = context.config.TC_AWS_LOADER_ROOT_PATH
	return self._s3_client

[heynemann]

I get why you added a class to make it a singleton but we can make a singleton just as easily with methods:

__instance = None

def load(params):
    if __instance is None:
        __instance = Whatever()

    # do something with __instance

[heynemann]

Much clearer! TYVM!