search

lindenlab / caddy-s3-proxy

s3 proxy plugin for caddy
Apache License 2.0
72 stars 22 forks source link

Close GetObject bodies to avoid leaking memory #70

Closed mcrute closed 3 months ago

mcrute commented 3 months ago

Calls to the GetObject method of the S3 client require that the Body be closed to avoid leaking memory. The returned struct is an http.Response returned by an http.Client (verified by digging into the AWS SDK dependency chain) and is guaranteed to always have a non-nil Body.

The only way I've been able to spot this issue is in prod after a few days of load. I'm testing this patch now and it already appears to be holding Caddy memory usage much more steady.

Related to and should fix #64.

Summary by CodeRabbit

coderabbitai[bot] commented 3 months ago

Walkthrough

The recent changes to the S3Proxy class in s3proxy.go focus on enhancing resource management by implementing defer statements to ensure that the bodies of S3 object responses are properly closed after use. This aims to prevent resource leaks and improve the application’s stability and performance, particularly in long-running processes.

Changes

Files Change Summary
s3proxy.go Added defer obj.Body.Close() in the serveErrorPage method and multiple instances in the GetHandler method to ensure proper closure of S3 object bodies.

Assessment against linked issues

Objective Addressed Explanation
High CPU usage after 32 days with caddy-s3-proxy (#64) ✅

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
mcrute commented 3 months ago

Eh... this change didn't really fix the issue. Although it also didn't make it worse so it might be worth merging. That being said I've migrated to caddy-fs-s3 instead so I'm going to withdraw this PR.