5 ms·

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

bsaul 6y ago
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
kitd 6y ago
Indeed. I like the push approach outlined here: http://marcio.io/2015/07/handling-1-million-requests-per-minute-with-golang/ 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.
- knorker 6y ago
  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 https://news.ycombinator.com/item?id=25831844
  - kitd 6y ago
    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.
  - rakoo 6y ago
    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 ?
    - knorker 6y ago
      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.
  - sagichmal 6y ago
    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.
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 ago
  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.
  - knorker 6y ago
    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.

bsaul 6y ago

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

kitd 6y ago

Indeed. I like the push approach outlined here: http://marcio.io/2015/07/handling-1-million-requests-per-minute-with-golang/ 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.

knorker 6y ago
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 https://news.ycombinator.com/item?id=25831844
- kitd 6y ago
  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.
- rakoo 6y ago
  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 ?
  - knorker 6y ago
    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.
- sagichmal 6y ago
  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.

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 ago
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.
- knorker 6y ago
  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.