Fix the RemotelyControlledSampler so that it terminates go-routine on Close() #260
Conversation
…o-routine terminate when Close() is called Signed-off-by: Scott Kidder <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
==========================================
+ Coverage 85.75% 86.25% +0.49%
==========================================
Files 54 55 +1
Lines 2850 2968 +118
==========================================
+ Hits 2444 2560 +116
- Misses 285 287 +2
Partials 121 121
Continue to review full report at Codecov.
|
| var wg sync.WaitGroup | ||
| wg.Add(1) | ||
| s.doneChan <- &wg | ||
| wg.Wait() |
There was a problem hiding this comment.
Looks good in general, is there an advantage to waiting for the poller update to complete? (Wouldn't it be gc'd with simply with the return on L493?)
There was a problem hiding this comment.
it's a pattern we used in several places. The main objective is to ensure that the poller GR is about to exit and is guaranteed to not do any work, for example by receiving another ticket update and performing sampler update. We may not need this guarantee here, but I would leave it for symmetry with the remote reporter.
If such guarantee was not needed, a simpler pattern is to close the doneChan and not wait for the GR to report readiness to exit.
There was a problem hiding this comment.
Thanks for the explanation!
From my understanding of select case, only one case can be selected at a time. Selecting the case with doneChan would result in the GR cleanly returning without doing any additional work and maintaining the guarantee, no?
Is symmetry at the cost of additional complexity important?
There was a problem hiding this comment.
select runs in a loop in a different GR from the one that calls Close(). Without a barrier Close may return prior to the loop GR exiting, so if any tests rely on having no updates after a call to Close we might break them.
I am not adding new complexity, the wait group was already in the implementation. I think my change makes that implementation simpler by localizing several vars that were previously spread out in different methods (like the ticket and the wait group itself). It also makes Close safe to call >1, something that we've seen people had trouble with in the reporter.
| } | ||
| } | ||
|
|
||
| func (s *RemotelyControlledSampler) getSampler() Sampler { |
There was a problem hiding this comment.
There was a problem hiding this comment.
not following - what is your point?
There was a problem hiding this comment.
/s/getSampler()/sampler() is the supposed "go way"
| manager: &httpSamplingManager{serverURL: options.samplingServerURL}, | ||
| doneChan: make(chan *sync.WaitGroup), |
There was a problem hiding this comment.
Interesting. Most other times we've done this, we created a empty chan (chan struct{}) and closed it inside Close() and had the pollControllerWithTicker intercept the closing of the chan and then calling s.wg.Done() vs sending the actual wg inside chan to the go routine. Both work but I've not seen it done this way anywhere inside our codebase.
There was a problem hiding this comment.
remote reporter uses similar pattern to this one, it sends a command to the channel and the command contains a WG. I think this is a better pattern than having WG as a member variable (leaking its scope unnecessarily).
Fixes #259
Signed-off-by: Scott Kidder [email protected]