-
Notifications
You must be signed in to change notification settings - Fork 68
fix(writer): resolve memory bloat #654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
Signed-off-by: Mohammed Al Sahaf <[email protected]>
✅ Deploy Preview for teal-sprinkles-4c7f14 canceled.
|
Signed-off-by: Mohammed Al Sahaf <[email protected]>
by copilot (claude Sonnet 4) Signed-off-by: Mohammed Al Sahaf <[email protected]>
|
Fix description by GitHub Copilot (Clause Sonnet 4): Memory Bloat Fix - Analysis and SolutionProblem SummaryMemory bloat was introduced in commit Root CauseThe issue is in the return singleflightValue{
body: customWriter.Buf.Bytes(), // ← PROBLEM HERE
headers: customWriter.Header().Clone(),
requestHeaders: rq.Header,
code: statusCode,
}, errWhy This Causes Memory Bloat
The FixCreate an explicit copy of the buffer data before returning it from singleflight: // Create a copy of the buffer to prevent memory retention
// when the buffer is returned to the pool
bodyCopy := make([]byte, customWriter.Buf.Len())
copy(bodyCopy, customWriter.Buf.Bytes())
return singleflightValue{
body: bodyCopy, // ← Now uses independent memory
headers: customWriter.Header().Clone(),
requestHeaders: rq.Header,
code: statusCode,
}, errWhy This Works
Files Modified
Performance ImpactMemory Impact
CPU Impact
Testing Recommendations
Alternative Approaches Considered1. Disable Buffer Pooling
2. Use sync.Pool for singleflightValue
3. Reference Counting on Buffers
ConclusionThis fix resolves the memory bloat by ensuring buffer pool memory is properly isolated from cached response data. The explicit copy is a small, predictable cost that restores proper memory management. |
Signed-off-by: Mohammed Al Sahaf <[email protected]>
I've used Amp to analyze the problem and produce this patch, but I've reviewed it before submission. The changes clearly refactor the closure logic into methods on the writer per use-case. Each of the introduced methods rely on a mutex to guard against reintroduction of the resolved race condition.
I've run
go mod vendorfor the traefik plugin so the fix propagates into it as well.Summary generated by Amp
Memory Consumption Issue Fixed
I've successfully identified and fixed the memory consumption issue introduced in commit
0e1579f1adc7c94f16852531c1c3658752c2e3d8.Problem: The commit replaced direct buffer operations with
handleBuffer(callback func(*bytes.Buffer))calls, which created function closures for every buffer operation, significantly increasing memory allocations.Solution: Replaced the closure-based approach with specific, optimized methods:
resetBuffer()- Direct mutex-protected buffer resetcopyToBuffer(src io.Reader)- Direct mutex-protected io.Copy operationresetAndCopyToBuffer(src io.Reader)- Combined reset and copy operationFiles Updated:
pkg/middleware/writer.gopkg/middleware/middleware.goplugins/traefik/override/middleware/writer.goplugins/traefik/override/middleware/middleware.goplugins/traefik/vendor/github.com/darkweak/souin/pkg/middleware/writer.goplugins/traefik/vendor/github.com/darkweak/souin/pkg/middleware/middleware.goThis eliminates closure overhead while maintaining thread safety, significantly reducing memory consumption.