3 ms·
The downside to that more "not idiomatic Go" example is that you lose useful stack traces, and it complicates contexts (timeouts, credentials, tracing, etc… etc
by knorker 6y ago
The downside to that more "not idiomatic Go" example is that you lose useful stack traces, and it complicates contexts (timeouts, credentials, tracing, etc… etc…).
For this reason the article I would say is also suboptimal, and is non-idiomatic. It's something you'd do in other languages, but should be avoided in Go.
In my opinion you should just start a goroutine. The goroutine could block on a semaphore/channel to limit concurrency, but there's nothing inherently more costly about having the goroutines themselves be the queue instead of like the article having a list.
Yes, it'll probably take a bit more memory to create a goroutine than to add to a list, but almost always I'll take that to get more correct behavior.
But also, your suggestion doesn't handle the requirement "Calling the service should not block the caller", does it?
The article overengineered, by far. You don't need infrastructure for this. It's just:
for work := range workGenerator() {
go process(work)
}
to limit concurrency, create a semaphore and just block either before starting goroutine (thereby blocking caller):
sem := sync.NewSemaphore(runtime.NumCPU())
for work := range workGenerator() {
work:=work
sem.Acquire(1)
go func() {
defer sem.Release()
process(work)
}()
}
Or in the goroutine, to not block the caller:
sem := sync.NewSemaphore(runtime.NumCPU())
for work := range workGenerator() {
work:=work
go func() {
sem.Acquire(1)
defer sem.Release()
process(work)
}()
}
You don't need the infrastructure from the article and, as I described, it's actually hurting.
- rthinker 6y agoNot blocking the caller is usually bad, the program can run out of memory if the producer is much faster than the consumers. Also, in more realistic scenarios "process" can error, one would want to use the errgroup package.
- knorker 6y agoI presented both solutions because "it depends". Though I agree with "usually". Yes, if in doubt then block the caller. I mainly provided it as an example because the article had it as an explicit requirement. You're right about errgroup. One can fit only so much in a HN comment. And I didn't want to distract by making it look like "no, you should use my favourite library instead". But again it depends. If an error is handled by doing log.Fatal(), then there's no point in using errgroup. I'm also not passing ctx, which much (most?) nontrivial code should pass.
- effectiveshroom 6y agoChannel itself is a good "semaphore" to restrict parallelism imho
- makeworld 6y agoWhere is that semaphore struct from? I can't find sync.NewSemaphore in "sync" or "golang.org/x/sync".
- Thaxll 6y agoThere is a more standard way of doing that with a blocking channel. c := make(chan struct{}, 10) for job := range jobs { c <- struct{}{} go func(){ defer func() { <-c }() }(job) }
- andrenth 6y agoI believe it's meant to be sem := semaphore.NewWeighted(int64(runtime.NumCPU())) from "golang.org/x/sync/semaphore"
- sprt 6y agoAgree, in fact this pattern is found almost verbatim in Go's documentation: https://pkg.go.dev/golang.org/x/sync/semaphore#example-package-WorkerPool https://pkg.go.dev/golang.org/x/sync/semaphore#example-packa...
- burnthrow 6y agoGo's Memory Model documentation specifically recommends modeling a counting semaphore with a buffered channel. https://golang.org/ref/mem#tmp_7 https://golang.org/ref/mem#tmp_7