fix: routine leak for dummy segments

nikolaianohyn commented 2 years ago

Issue 364

If segment is dummy don't create go routine for context cancelation handling

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

NathanielRN commented 2 years ago

Thanks for this PR @nikolaianohyn! I agree that this should fix it 🙂

However, can we still incorporate @sawadashota's solution here? I think it's good to be safe even if we don't add the cancelCtx to the segment anymore.

i.e. changing this code:

https://github.com/aws/aws-xray-sdk-go/blob/b4b13727e8c47e1c3227958df028be4cd823d3d7/xray/segment.go#L360-L374

To look like this:

cancelSegCtx := seg.cancelCtx

seg.Unlock()

// If a goroutine was started to close the extra context (made for
// cancelling the segment), stop the goroutine before we close the segment
// and lose access to it.
if cancelSegCtx != nil {
    cancelSegCtx()
}

// If segment is dummy we return
if seg.Dummy {
    return
}

seg.send()

I don't see any precedence on testing Dummy segments so I won't ask for it in this PR, but if you were interested in trying to make one I'd be happy to review it 😄

NathanielRN commented 2 years ago

The unit tests are failing but I don't believe the changes from this PR are related. We've seen tests fail on re-run because of global state before so I'll merge this PR despite the failing tests because the have passed before with the original change.

aws / aws-xray-sdk-go

fix: routine leak for dummy segments #365