Persist S3 client through Bucket __init__

[jimas14]

Our team was tasked to upgrade Thumbor (from 6.7.0) to be able to preserve IPTC metadata. This requires a custom engine https://github.com/scorphus/thumbor-wand-engine, which requires python 3, which requires Thumbor 7.0.0a5. After putting all the pieces together, including the code from @amanagr's PR thumbor-community#147 to accommodate async Thumbor 7, things worked, but we noticed a ton of asyncio errors and a minimum response time of > 3000ms on load testing.

We utilize S3 loader, loader storage & result storage for our requests, and we noticed that the S3 client creation/Bucket initialization was being called upwards of 5 times for each request.

We took a pass at trying to improve this by only creating the S3 client on Bucket's first init call, and reusing that client for every subsequent request. These changes alone took the minimum response time down to < 200ms (like our 6.7.0 implementation did), and performed very well in load tests (15 containers, numprocs=2, 1000 random requests by 10 users -- same parameters as first load test).

Update (7/14): After noticing huge latency spikes with this code in production, we took another pass at optimization. Testing locally I saw that with N number of consecutive requests, processing would get blocked and lead to a ~45 second response. After analyzing the code again I realized that Bucket.exists was never actually awaiting the response['Body'].read() call from the storage.get response, so the stream was never closed, leading to lots of asyncio Unclosed response errors (see aio-libs/aiobotocore#68). I updated the exists method to just drop the storage.get altogether in favor of using HeadObject which is just metadata, no streams to be read from. Then I updated the rest of the response['Body'].read() calls to be wrapped in a try/with block so it's more resource-safe. I also removed the extra changes here which @kkopachev recommended leaving out.

I'm very interested to see what you all think. There was a couple other small refactors added as well. Credits to @cselvaraj for the idea & implementation.

cc @amanagr @kkopachev

A couple explanations:

• The name change from Storage -> S3ResultStorage class was purely for easier identification in the profiler.
• The broad except Exception in s3_storage.py was added because the NoSuchKey exception wasn't being caught by ClientError or BotoCoreError, and I couldn't find where NoSuchKey exception actually lives.

[kkopachev]

so, from what I understand, this is the only required change here. Other changes are rather cosmetic.

[cselvaraj]

Yes, this is the operative change. The rest are PEP8 changes and very minor performance changes.

[kkopachev]

This looks good. I'd only keep Bucket.__init__ changes as most relevant.

[kkopachev]

Ultimate solution to this trouble would be to modify Thumbor so it is able to call preinit methods on storage and other modules. And cleanup right before server shutdown.

[jimas14]

@kkopachev Thanks so much for the review. I just updated the PR if you could take another look. I updated the PR description with our updated findings.

[kkopachev]

This is awesome @jimas14! Great catch on HeadObject operation!
My only concern here is that we're pinning aibotocore to an older version.

[peterrus]

Potentially relevant: thumbor-community#147 (comment)