Hacker News Reader

Comment by weddpros | Hacker News Reader

I would start n goroutines select'ing on the same bounded channel, which I find much simpler... or did I miss something?

View on HN

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.

Not 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.

I 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.

Where is that semaphore struct from? I can't find sync.NewSemaphore in "sync" or "golang.org/x/sync".

There 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)
  }

I believe it's meant to be

    sem := semaphore.NewWeighted(int64(runtime.NumCPU()))

from "golang.org/x/sync/semaphore"

Agree, in fact this pattern is found almost verbatim in Go's documentation: https://pkg.go.dev/golang.org/x/sync/semaphore#example-packa...

Go's Memory Model documentation specifically recommends modeling a counting semaphore with a buffered channel.

https://golang.org/ref/mem#tmp_7

Indeed.

I like the push approach outlined here:

http://marcio.io/2015/07/handling-1-million-requests-per-min...

I have done a more generic version of this with success.

You have N workers, each reading from a separate input channel for work to do.

Then you have an outer "channel of channels", holding those worker channels. When you want to submit work, select a worker channel from the outer channel, submit, and push the worker channel back on the outer channel when done.

Core people in Go have enlightened me that the "thread worker" model in Go is non-idiomatic.

It breaks contexts (and all that implies, like credentials, tracing, timeouts) and stacks (making it harder to debug).

You should just start the goroutine instead of enqueueing it.

More info here: https://news.ycombinator.com/item?id=25831844

I agree, it may break contexts, but the "idiomatic" form is mentioned in the article I linked and was found much less scalable than the final version.

It's all tradeoffs, I guess.

I guess it's because on one hand the context is handled by the function directly (with the usual `process(ctx, job)` pattern) and on the other hand the context needs to be part of the job, which is less idiomatic ? Functions vs Types. Interesting.

The issue with spawning goroutines all the time is that spawning, while cheap individually, can become expensive if done in large numbers (like 1 Million times a minute as in the article). Is that not considered problematic ?

Yeah, in the longer comment I said that it "complicates" contexts, because you can put the context in the work unit. But it's very easy to get it wrong.

E.g. the godoc for the context package itself says "Do not store Contexts inside a struct type". I understand the reason to be that it's easy to make the lifetime nonobvious, and make it "strange" (hard to read and reason about) if it's not very strictly kept under control. In other words it becomes a foot-gun as the code base grows.

And even then the stack is still unhelpful.

Benchmarking for your own workload beats anything else no matter what the language, of course. But it may change in the future, too. Maybe the next version of Go actually makes it faster? In other words it's not wrong, but great care should be used.

One reason it can be faster to spawn goroutines is that IIRC spawning a goroutine actually makes the current OS thread start running that. And if it completes then it can jump back to the spawner. In other words spawning goroutine can save thread creations and context switches, while a goroutine pool will likely incur an OS context switch, with cache implications and other complex interactions.

But yes, measure with your actual workload is king.

It's not necessarily idiomatic or nonidiomatic, it's just a pattern that's appropriate in some circumstances and not appropriate in others.

If you want best-effort asynchronous job processing semantics, then it's totally appropriate to use something like what's described in the article -- ideally with a lot less code :)

If you want request-response, then, yeah, this isn't appropriate, and I agree that you should do whatever work in the request goroutine, blocking as necessary.

agreed.. separating the signal from the request queue seems like an anti-pattern... Not sure who the author is, and why this has reached front page so we probably missed something

Reply on news.ycombinator.com