aws / aws-xray-sdk-go

AWS X-Ray SDK for the Go programming language.
Apache License 2.0
276 stars 117 forks source link

xray: Don't leak goroutines on segment start #327

Closed narqo closed 2 years ago

narqo commented 3 years ago

Issue #, if available: This is a follow-up for https://github.com/aws/aws-xray-sdk-go/issues/51 and https://github.com/aws/aws-xray-sdk-go/pull/156

Description of changes: https://github.com/aws/aws-xray-sdk-go/pull/156 has fixed a leak caused by the use of context.Background() with xray.BeginSegmentWithSampling. This won't work if a service's background job, uses a context, whose cancellation is bound to service's life-time. E.g. consider the following code:

func main() {
    ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
    defer stop()

    ticker := time.NewTicker(time.Minute)
    defer ticket.Stop()
    for {
        select {
        case <-ticker.C:
            _, seg := xray.BeginSegment(ctx, "")  // 👈 spawns a goroutine that never stops
            // do something
            seg.Close(nil)
        case <-ctx.Done():
            break
        }
    }
}

This PR aims at fixing the issue by making sure the goroutine, that xray.BeginSegment spawns will be stopped after the segment gets closed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

narqo commented 2 years ago

🆙

@bhautikpip I've push the updated version, PHAL

narqo commented 2 years ago

@bhautikpip, I've noticed that the CI went red after your approval. Apparently, some unit tests in the package relied on the global state. So the unit-test, that I'd added earlier started to interfere with the old ones. I've added one more commit to the PR to make at least my tests better 🙏

narqo commented 2 years ago

@bhautikpip is there anything blocking from merging this PR, or anything I could help to move this forward?