Skip to content

Conversation

@mohammed90
Copy link
Contributor

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 vendor for 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 reset
  • copyToBuffer(src io.Reader) - Direct mutex-protected io.Copy operation
  • resetAndCopyToBuffer(src io.Reader) - Combined reset and copy operation

Files Updated:

  • pkg/middleware/writer.go
  • pkg/middleware/middleware.go
  • plugins/traefik/override/middleware/writer.go
  • plugins/traefik/override/middleware/middleware.go
  • plugins/traefik/vendor/github.com/darkweak/souin/pkg/middleware/writer.go
  • plugins/traefik/vendor/github.com/darkweak/souin/pkg/middleware/middleware.go

This eliminates closure overhead while maintaining thread safety, significantly reducing memory consumption.

@netlify
Copy link

netlify bot commented Aug 1, 2025

Deploy Preview for teal-sprinkles-4c7f14 canceled.

Name Link
🔨 Latest commit 8911ffd
🔍 Latest deploy log https://app.netlify.com/projects/teal-sprinkles-4c7f14/deploys/68ddafd17fe04b0007941de8

@mohammed90
Copy link
Contributor Author

Fix description by GitHub Copilot (Clause Sonnet 4):

Memory Bloat Fix - Analysis and Solution

Problem Summary

Memory bloat was introduced in commit 0e1579f1adc7c94f16852531c1c3658752c2e3d8 which attempted to fix a buffer race condition. While the race condition fix worked, it inadvertently caused memory to grow unbounded.

Root Cause

The issue is in the Upstream() and Revalidate() functions where singleflightValue structs are returned from the singleflight pattern:

return singleflightValue{
    body:           customWriter.Buf.Bytes(),  // ← PROBLEM HERE
    headers:        customWriter.Header().Clone(),
    requestHeaders: rq.Header,
    code:           statusCode,
}, err

Why This Causes Memory Bloat

  1. customWriter.Buf.Bytes() returns a slice that references the underlying buffer array

    • This is NOT a copy - it's just a slice header pointing to the buffer's internal memory
  2. The buffer is returned to the pool via defer s.bufPool.Put(bufPool)

    • The pool reuses this buffer for future requests
  3. The old singleflightValue.body slice still references the original buffer memory

    • Even though the buffer is back in the pool and being reused
    • The garbage collector cannot free the buffer because the slice keeps it alive
  4. Each request creates a new slice referencing buffer memory

    • Memory accumulates over time as buffers grow to accommodate larger responses
    • Old slices prevent garbage collection of buffer memory

The Fix

Create 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,
}, err

Why This Works

  1. make([]byte, len) allocates new, independent memory
  2. copy() copies the actual bytes into the new allocation
  3. The bodyCopy slice has no connection to the pooled buffer
  4. When the buffer returns to the pool, it can be fully reused
  5. The GC can free bodyCopy when the singleflightValue is no longer referenced

Files Modified

  1. pkg/middleware/middleware.go - Main middleware (2 locations)
  2. plugins/traefik/override/middleware/middleware.go - Traefik override (2 locations)
  3. plugins/traefik/vendor/github.com/darkweak/souin/pkg/middleware/middleware.go - Traefik vendor copy (2 locations)

Performance Impact

Memory Impact

  • Before: Memory grows unbounded as buffer pool becomes ineffective
  • After: Normal memory usage with proper buffer pool reuse

CPU Impact

  • Minor increase: Each request performs one additional memory allocation and copy
  • Trade-off: This is negligible compared to the I/O costs and prevents memory exhaustion
  • Net positive: Avoiding OOM crashes and memory pressure on GC far outweighs the copy cost

Testing Recommendations

  1. Load test with sustained traffic to verify memory is stable
  2. Monitor metrics:
    • Heap allocation rate
    • GC pause times
    • Buffer pool statistics (if exposed)
  3. Compare memory profiles before and after the fix using pprof

Alternative Approaches Considered

1. Disable Buffer Pooling

  • Rejected: Would hurt performance significantly
  • Pooling is essential for high-throughput scenarios

2. Use sync.Pool for singleflightValue

  • Rejected: Complexity doesn't justify the benefit
  • The singleflightValue lifetime is short and managed by singleflight

3. Reference Counting on Buffers

  • Rejected: Overly complex and error-prone
  • Go's GC handles memory management well once we break the retention

Conclusion

This 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants