CODEWITHSUNDEEP
gogoroutine
ANKUR KUMAR
ANKUR KUMAR

Reputation: 33

Golang worker pool implementation working unexpectedly

I have implemented a golang worker pool as below where sem and work are channels. sem is a channel to keep track of number of workers(goroutines) currently active. work is channel to pass functions to active workers to execute. timeout will return any worker idle for the timeout duration.

package main

import (
    "time"
)

type Pool struct {
    sem chan struct{}
    work chan func()
    timeout time.Duration
}

func NewPool(max, size, spawn int, timeout time.Duration) *Pool {
    if spawn <= 0 {
        panic("workpool spawn is <= 0")
    }
    if spawn > max {
        panic("workpool spawn > max workers")
    }
    p := &Pool{
        sem: make(chan struct{}, max),
        work: make(chan func(), size),
        timeout: timeout,
    }
    for i := 0; i < spawn; i++ {
        p.sem <- struct{}{}
        go p.worker(func() {})
    }
    return p

}

func (p *Pool) AddTask(task func()) {
    select {
        case p.work <- task:
            return
        case p.sem <- struct{}{}:
            go p.worker(task)
            return
    }
}

func (p *Pool) worker(task func()) {
    t := time.NewTimer(p.timeout)
    defer func() {
        t.Stop()
        <- p.sem
    }()

    task()

    for {
        select {
            case task := <- p.work:
                t.Reset(p.timeout)
                task()
            case <- t.C:
                return
        }
    }
}

I am testing by printing the value of i in a for loop by passing it into the pool wrapped in an anonymous function as below:

package main

import (
    "fmt"
    "time"
)

func main() {
    fmt.Println("Hello, world!")

    p := NewPool(3, 10, 1, time.Duration(5) * time.Second)

    for i:=0; i<30; i++ {
        p.AddTask(func () {
            fmt.Print(i, " ")
        })
    }
    time.Sleep(10 * time.Second)
    fmt.Println("End")

}

The expected output should be serial numbers from 0 to 29 but instead output is

Hello, world!
12 12 12 12 12 12 12 12 12 12 12 12 13 25 25 25 25 25 25 25 25 25 25 25 26 25 30 30 30 30 End

I cannot understand why the output is like the above.

Upvotes: 2

Views: 918

Answers (1)

colm.anseo
colm.anseo

Reputation: 22037

Your function closures are all referencing the same value of i. This creates a race condition, as the functions are dispatched, they are reading a changing value - hence the unpredictable output you are seeing.

To ensure closure gets a unique value, declare the variable within the loop. A simple trick to do this is by shadow declaring the same variable name i := i

for i:=0; i<30; i++ {
    i:= i                // <- add this
    p.AddTask(func () {
        fmt.Print(i, " ")
    })
}

https://go.dev/play/p/o0Nyx5A46tp

BTW this technique is covered in the Effective Go docs, see this section:

It may seem odd to write

req := req

but it's legal and idiomatic in Go to do this. You get a fresh version of the variable with the same name, deliberately shadowing the loop variable locally but unique to each goroutine.

Upvotes: 2

Related Questions