Skip to content
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

[BUG] goroutine leak of datastreams.Processor from mocktracer #2858

Closed
ggambetti opened this issue Sep 12, 2024 · 4 comments · Fixed by #2880
Closed

[BUG] goroutine leak of datastreams.Processor from mocktracer #2858

ggambetti opened this issue Sep 12, 2024 · 4 comments · Fixed by #2880
Assignees
Labels
bug unintended behavior that has to be fixed

Comments

@ggambetti
Copy link
Contributor

Version of dd-trace-go

Visible on v1.66.0 although from a read through of code on main the issue is still present.

Describe what happened:

  1. Create a test that uses mocktracer + https://github.com/uber-go/goleak
  2. defer goleak.VerifyNone(t) + defer tracer.Stop()
  3. run application code
  4. Note leaked goroutines:
    request_test.go:122: found unexpected goroutines:
        [Goroutine 8 in state chan receive, with gopkg.in/DataDog/dd-trace-go.v1/internal/datastreams.(*Processor).reportStats on top of the stack:
        gopkg.in/DataDog/dd-trace-go.v1/internal/datastreams.(*Processor).reportStats(0x14000166540)
                /Users/giannigambetti/go/pkg/mod/gopkg.in/!data!dog/[email protected]/internal/datastreams/processor.go:375 +0x7c
        created by gopkg.in/DataDog/dd-trace-go.v1/internal/datastreams.(*Processor).Start in goroutine 7
                /Users/giannigambetti/go/pkg/mod/gopkg.in/!data!dog/[email protected]/internal/datastreams/processor.go:343 +0x108
         Goroutine 9 in state sleep, with time.Sleep on top of the stack:
        time.Sleep(0x989680)
                /opt/homebrew/Cellar/go/1.23.1/libexec/src/runtime/time.go:285 +0xe0
        gopkg.in/DataDog/dd-trace-go.v1/internal/datastreams.(*Processor).run(0x14000166540, 0x140001da770)
                /Users/giannigambetti/go/pkg/mod/gopkg.in/!data!dog/[email protected]/internal/datastreams/processor.go:327 +0x18c
        gopkg.in/DataDog/dd-trace-go.v1/internal/datastreams.(*Processor).Start.func1()
                /Users/giannigambetti/go/pkg/mod/gopkg.in/!data!dog/[email protected]/internal/datastreams/processor.go:349 +0x90
        created by gopkg.in/DataDog/dd-trace-go.v1/internal/datastreams.(*Processor).Start in goroutine 7
                /Users/giannigambetti/go/pkg/mod/gopkg.in/!data!dog/[email protected]/internal/datastreams/processor.go:345 +0x158
        ]

A read through the code shows that (*Processor).reportStats is always going to leak: https://github.com/DataDog/dd-trace-go/blob/main/internal/datastreams/processor.go#L374, as there's no mechanism to clean it up.

But (*Processor).run won't leak if the (*Processor).Stop is called: https://github.com/DataDog/dd-trace-go/blob/main/internal/datastreams/processor.go#L366-L372, https://github.com/DataDog/dd-trace-go/blob/main/internal/datastreams/processor.go#L320.

Describe what you expected:

The call to mocktracer.Stop() explicitly shuts down the Processor it started, and blocks to wait for all spawned goroutines to exit.

Steps to reproduce the issue:

package test

import (
  "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer"
  "go.uber.org/goleak"
)

func TestMocktracerLeaks(t *testing.T) {
  defer goleak.VerifyNone(t)
  mt := mocktracer.Start()
  defer mt.Stop()
}

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

OS: macOS 14.5 (23F79) (Sinoma + M1)
GO: go version go1.23.1 darwin/arm64

@ggambetti ggambetti added the bug unintended behavior that has to be fixed label Sep 12, 2024
@github-actions github-actions bot added the needs-triage New issues that have not yet been triaged label Sep 12, 2024
@ggambetti
Copy link
Contributor Author

I've staged a prototype fix: main...ggambetti:dd-trace-go:main, but haven't made a pull request because I can't get the project to build off of main.

(Apparently the cause is chenzhuoyu/iasm#2?)

@darccio darccio removed the needs-triage New issues that have not yet been triaged label Sep 17, 2024
@darccio
Copy link
Member

darccio commented Sep 17, 2024

Thanks for detecting and contributing the fix, @ggambetti. Were you able to build off of main using setting GOPROXY according to this comment in the linked issue?

I skimmed the fix and it looks good.

@darccio darccio self-assigned this Sep 17, 2024
@ggambetti
Copy link
Contributor Author

I put this down for the weekend and hadn't yet revisited.

I've allocated some time to this over the next few days to get the PR into a shippable state. If folks don't see any follow up from me before the 23rd (2024/09/23) feel free to take it over. 😄

@ggambetti
Copy link
Contributor Author

I managed to get the PR building & tests running, and gotten it into an almost shippable state: #2880

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants