Closed andydunstall closed 11 months ago
You probably told me this but I do not remember now. Are we creating a connection for each request and then we close it?
Are we creating a connection for each request and then we close it?
Yeah - not great but given the HTTP client is accessed by multiple proactors seemed like its easiest just to keep it stateless (we don't have much control over access given the AWS SDK has a global static HTTP client - can improve later when removing the SDK)
Given each S3 operation is an 8MB upload/download the connection overhead is minimal (I ran tests using s3_demo
using a single connection vs a connection per request and there wasn't much difference)
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
077a00d
) 77.50% compared to head (d348ff3
) 77.56%.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Adds HTTPS support to the AWS S3 client. (Will also add a dragonfly option to disable/enable HTTPS)
This uses the existing TLS client context and setup (since the AWS HTTP client writes directly to the socket, using
TlsClient
itself would be a pain so it just copies what it needs).Since the HTTPS client is shared by multiple proactors, this still doesn't do connection caching. This has a bigger overhead with HTTPS than HTTP though is still negligible compared to the 8MB chunk uploads and downloads so I don't think thats an issue (users can disable HTTPS for S3 endpoints if preferred).
(Will add connection caching if/when replacing the AWS SDK and add a separate HTTP client per proactor rather than a global shared client)
TLS Not all AWS endpoints support TLS 1.3, though all support TLS 1.2, therefore updates the SSL context min version to 1.2 (down from 1.3)