Simple multiple producer and single consumer concurrency pattern

Question

Anyone please review and share your feedback for the following code. Multiple producers generate random integer numbers and a single consumer receives it to display only odd numbers out of it.

package main

import (
"fmt"
"time"
"math/rand"
"sync"
)

var msgs = make(chan int)
var dummy = make(chan bool)
var wg sync.WaitGroup

func main() {
    defer close(msgs)
    defer close(dummy)
    numberOfProducers := 10
    go func() {
        for i := 0; i < numberOfProducers ; i++ {
            go produce() // Multiple producers
        }
        dummy <- true
    }()
    go consume() // Single consumer go routine
    <- dummy
}

func produce() {
     wg.Add(1)
     msgs <- getRandomIntegerNumber()
}

// Single consumer
func consume () {
    for {
      msg := <-msgs
      if ( msg % 2 != 0) {
         fmt.Printf(" The number %d is odd \n", msg);
      }
      wg.Done()
    }
}

func getRandomIntegerNumber() int {
     rand.Seed(time.Now().UnixNano())
     return rand.Intn(1000)
}

Answer 1

So currently the WaitGroup is not being used to wait for the producing to complete. This means that it is possible for this programme to run without producing any results as each go produce() is done in it's own go routine so true can be sent immediately to dummy if the main go routine gets all the CPU time thus main exits with nothing being printed.

Personally I would use the WaitGroup to do the waiting and then close the msgs channel to indicate to the consume that it is done and use a further WaitGroup to then indicate to main that consume is done.

In addition to the concurrency parts, I also wouldn't use global variables but instead pass them into the functions that use them. Putting this together then you get:

package main

import (
    "fmt"
    "math/rand"
    "sync"
    "time"
)

func main() {
    msgs := make(chan int)

    // Start the consuming go routine before producing so that we can then just wait for the producing
    // to complete on the main go routine rather than having to have a separate go routine to wait to
    // close the channel.
    var consumeWG sync.WaitGroup
    consumeWG.Add(1)
    go consume(msgs, &consumeWG)

    var produceWG sync.WaitGroup
    numberOfProducers := 10
    for i := 0; i < numberOfProducers; i++ {
        produceWG.Add(1)
        go produce(msgs, &produceWG) // Multiple producers
    }

    // Wait for producing to complete then tell the consumer by closing the channel.
    produceWG.Wait()
    close(msgs)

    consumeWG.Wait()
}

func produce(msgs chan int, wg *sync.WaitGroup) {
    defer wg.Done()
    msgs <- getRandomIntegerNumber()
}

// Single consumer
func consume(msgs chan int, wg *sync.WaitGroup) {
    // Range of msgs, this will consume all messages in the channel then exit when the channel is closed.
    // This provides communication between the go routines when completion has happened as well as not
    // leaking the consuming go routine as would happen with an forever loop.
    for msg := range msgs {
        if msg%2 != 0 {
            fmt.Printf(" The number %d is odd \n", msg)
        }
    }
    wg.Done()
}

func getRandomIntegerNumber() int {
    rand.Seed(time.Now().UnixNano())
    return rand.Intn(1000)
}

Answer 2

• First thing I found weird was the use of globals for the channels. This is generally a bad idea and rarely necessary.
• Then, in main() there is this dummy channel. It seems to be used as synchronization mechanism without any actual info transported through it. However, it's unclear what the intention is. As it stands, you only make sure that the goroutines with the producers were started before the program is allowed to terminate. What is the intention?
• Further, I wonder why you start the producers from a goroutine and not from a simple loop in main(). This may be required for something, but it is unclear to the casual reader of your program. That reader could be you in a few weeks when you forgot why you did that. Documenting the "why?" of your programs is the most valuable information.
• The WaitGroup wg, what is that for? You have added another synchronization mechanism, but it's similarly unclear what that is used for.
• You seed the RNG every time you retrieve a random number. That's bad, in particular since the behaviour of the seed value you use is very easily predictable. Seed it only once on program start.
• You usually only need one channel, shared between producers and consumer. I'm not sure if your requirements imply some other synchronization or perhaps a "finished" detection. You could perhaps solve that by closing the channel or sending a signal value, without adding more parts and making things even more complex.

Note: Your requirements are pretty unclear. Getting random numbers in a particular order is unlikely, but maybe that's just the example. If you provided more info what is produced and consumed, people could give you a better suggestion.