DataDog / dd-trace-go

Datadog Go Library including APM tracing, profiling, and security monitoring.
https://docs.datadoghq.com/tracing/
Other
675 stars 438 forks source link

[BUG] contrib/valyala/fasthttp.v1 memory leak #2961

Closed 0angelic0 closed 4 weeks ago

0angelic0 commented 4 weeks ago

The implementation of contrib/valyala/fasthttp.v1 WrapHandler has a memory leak issue. We use this in our production system and it cause memory leak. After profiling and review the code we found the issue is in the WrapHandler function.

Version of dd-trace-go

v1.69.0

Describe what happened:

The issue is the spanOpts variable in WrapHandler function. Now the code initialize the slice of tracer.StartSpanOption as WrapHandler function scope and use it in a closure anonymous function that will return as a fasthttp.RequestHandler. The usage is to be appended with the defaultSpanOptions and tracer context (from tracer.Extract and tracer.ChildOf). After that the spanOpts is used to create a Span.

The problem is now spanOpts is the same slice for every http request that will come in and be handled. So. the slice will growth indefinitely since every request will append several tracer.StartSpanOption into the same slice. And eventually causes the memory leak problem.

Describe what you expected:

Expect the spanOpts to not growing every time the request came in. So it doesn't cause a memory leak.

It should be initialized in the closure anonymous function that will return as a fasthttp.RequestHandler instead of WrapHandler function. So it will be garbage collected every time the request handler end.

Steps to reproduce the issue:

Just write a small hello world fasthttp program and load test it to see the memory leak.

This is the memory usage for 7 times of 10,000 requests each.

Image

Additional environment details (Version of Go, Operating System, etc.):

0angelic0 commented 4 weeks ago

I'm proposing the fix as to move the initialization of spanOpts into the closure anonymous function that will return as a fasthttp.RequestHandler. Already tested in my fork and the memory leak was fixed.

This is the memory usage for 7 times of 10,000 requests each. No memory leak.

Image