fix: routine leak for dummy segments

[nikolaianohyn]

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]

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:

aws-xray-sdk-go/xray/segment.go

Lines 360 to 374 in b4b1372

	// If segment is dummy we return
	if seg.Dummy {
	seg.Unlock()
	return
	}
	cancelSegCtx := seg.cancelCtx
	seg.Unlock()
	seg.send()
	// makes sure the goroutine, that waits for possible top-level context cancellation gets closed on segment close
	if cancelSegCtx != nil {
	cancelSegCtx()
	}

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]

I think we can improve on the original wording, Does this makes sense to you? Let me know!

Suggested change

	// don't allocate resources for dummy segments
	if !seg.Dummy {
	// create a new context to close it on segment close;
	// this makes sure the closing of the segment won't affect the client's code, that uses the returned context
	// Dummy segments don't get sent and don't need a goroutine to cancel them.
	if !seg.Dummy {
	// Create a new context for to cancel segment.
	// Start new goroutine to listen for segment close/cancel events.
	// Cancel simply calls `segment.Close()`.
	//
	// This way, even if the client consumes the `ctx.Done()` event, the
	// X-Ray SDK has a way of canceling (closing) the segment in the
	// `segment.Close()` method using this new cancellation context.

[NathanielRN]

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.