Fix race between startup and shutdown in task reader #5522
Conversation
Nothing ensures all goroutines are started before shutdown, and in tests that's somewhat likely to occur.
Risk to production is probably very low, but definitely best to fix there too.
As a general statement for any readers:
```go
func (s *self) thing() {
s.wg.Add(1)
defer s.wg.Done()
...
}
// elsewhere
go s.thing()
```
^ this pattern is *almost always* unsafe, at least on a technical level.
All adds need to occur before any Wait() can possibly finish, so they should **always** be added in the "parent" thread, never in the goroutine that things will wait on.
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| tr.stopWg.Add(1) | ||
| go func() { | ||
| defer tr.stopWg.Done() | ||
| tr.dispatchBufferedTasks(g) | ||
| }() |
There was a problem hiding this comment.
there are other ways to achieve this, but tbh I'm fond of "always make concurrency controls visible where it is made concurrent". it simplifies the method doing things, and makes "you added go, did you take care of X?" questions trivial.
alternatively, a backgroundDispatch(g) that does this internally (and you do not go backgroundDispatch(g)) works pretty well. that also makes you relatively immune to mutating range vars.
yea. tbh this is my biggest complaint with Go concurrency: it's "intrusive". you essentially must always put concurrency controls in by hand, because the whole system prevents you from building both robust and performant abstractions around it like Java has had for many years. Even just stuff like "there is no thread.Join" causes a LOT of error-prone needs like this. Generics can largely address that now, but there's a massive pile of history that will take a long time to upgrade / will influence habits for probably the next decade :\ |
Nothing ensures all goroutines are started before shutdown, and in tests that's somewhat likely to occur.
Risk to production is probably very low, but definitely best to fix there too.
As a general statement for any readers:
^ this pattern is almost always unsafe, at least on a technical level.
All adds need to occur before any Wait() can possibly finish, so they should always be added in the "parent" thread, never in the goroutine that things will wait on.