beacon,console,core,eth,p2p,rpc: keep wg.Add and wg.Done closer
#29818
Conversation
Halimao
requested review from
fjl,
holiman,
zsfelfoldi,
karalabe and
rjl493456442
as code owners
| @@ -47,7 +47,10 @@ func startEngineClient(config *lightClientConfig, rpc *rpc.Client, headCh <-chan | |||
| cancelRoot: cancel, | |||
| } | |||
| ec.wg.Add(1) | |||
| go ec.updateLoop(headCh) | |||
| go func() { | |||
| defer ec.wg.Done() | |||
There was a problem hiding this comment.
I don't really see the difference between two approaches. They should be equivalent, isn't it?
There was a problem hiding this comment.
@rjl493456442 Yes sir, in most cases they are equivalent.
But keeping wg.Done close to wg.Add is clearer for us to ensure we call wg.Done correctly.
And if we just wanna run ec.updateLoop in the main process for a test, wrapping wg.Done in ec.updateLoop will force us to call wg.Add even though I don't wanna run in a goroutine.
IMO letting the caller decide the way to call ec.updateLoop is more flexible.
There was a problem hiding this comment.
I think so too, I think if we follow the previous behavior, we must run this function in a goroutine
|
It's not a bad idea to structure the WaitGroup usage like it is proposed here, but there is no need to update all code retroactively to change the style. It works as is. |
Similar to #29813, this PR modifies all other
sync.WaitGroupcases.wg.Donewithoutwg.Addwill cause panic.wg.Done()is meaningless withoutwg.Addand goroutinewg.Addandwg.Donetogether can makes us easily ensure that both counts are equal.