Eng Ver. Common mistakes with for loops in Go

For loop + Goroutine + Closure

The program provided has the following code:

func main() {
    done := make(chan bool)

    values := []string{"a", "b", "c"}
    for _, v := range values {
        go func() {
            fmt.Println(v)
            done <- true
        }()
    }

    // wait for all goroutines to complete before exiting
    for _ = range values {
        <-done
    }
}

At first glance, one would expect the output to be "a, b, c". However, due to the unpredictable execution of Goroutines, they might all end up printing "c" instead of iterating through each value. This happens because Goroutines might execute after the next loop iteration, and the value of "v" might change by then.

Furthermore, the loop variable "v" in Go's for loop is shared, thus they all point to the same memory address. To prove this, there's another code segment:

func main() {
    done := make(chan bool)

    values := []string{"a", "b", "c"}
    for _, v := range values {
        go func() {
            fmt.Printf("value=%s, addr=%p\n", v, &v)
            done <- true
        }()
    }

    // wait for all goroutines to complete before exiting
    for _ = range values {
        <-done
    }
}

/*
value=c, addr=0xc000014070
value=c, addr=0xc000014070
value=c, addr=0xc000014070
*/

There are several solutions to this issue:

Make a copy of the value during each iteration.

 func main() {
     done := make(chan bool)

     values := []string{"a", "b", "c"}
     for _, v := range values {
         v1 := v
         go func() {
             fmt.Println(v1)
             done <- true
         }()
     }

     // wait for all goroutines to complete before exiting
     for _ = range values {
         <-done
     }
 }

Pass the value as a parameter to the closure function, thus creating a copy of the value.

 func main() {
     done := make(chan bool)

     values := []string{"a", "b", "c"}
     for _, v := range values {
         go func(v string) {
             fmt.Println(v)
             done <- true
         }(v)
     }

     // wait for all goroutines to complete before exiting
     for _ = range values {
         <-done
     }
 }

Go Tool Vet

This is a common interview question. Luckily, Go provides a static code analysis tool called "go vet". The tool checks for various issues, including the one with loop variables in nested functions. By running "go vet", one can identify potential issues like this.

To list the available checks, run "go tool vet help":
* asmdecl      report mismatches between assembly files and Go declarations
* assign       check for useless assignments
* atomic       check for common mistakes using the sync/atomic package
* bools        check for common mistakes involving boolean operators
* buildtag     check that +build tags are well-formed and correctly located
* cgocall      detect some violations of the cgo pointer passing rules
* composites   check for unkeyed composite literals
* copylocks    check for locks erroneously passed by value
* httpresponse check for mistakes using HTTP responses
* loopclosure  check references to loop variables from within nested functions
* lostcancel   check cancel func returned by context.WithCancel is called
* nilfunc      check for useless comparisons between functions and nil
* printf       check consistency of Printf format strings and arguments
* shift        check for shifts that equal or exceed the width of the integer
* slog         check for incorrect arguments to log/slog functions
* stdmethods   check signature of methods of well-known interfaces
* structtag    check that struct field tags conform to reflect.StructTag.Get
* tests        check for common mistaken usages of tests and examples
* unmarshal    report passing non-pointer or non-interface values to unmarshal
* unreachable  check for unreachable code
* unsafeptr    check for invalid conversions of uintptr to unsafe.Pointer
* unusedresult check for unused results of calls to some functions

You can view the usage instructions with "go tool vet help". Let me use "go tool vet" to scan and see.

go vet main.go
or
go vet -loopclosure main.go

/*
# command-line-arguments
./main.go:11:38: loop variable v captured by func literal
./main.go:11:42: loop variable v captured by func literal
*/

Seeing the message "loop variable v captured by func literal" alerts you that in line 11, the loop variable "v" is shared and is being used by the closure function. Once we modify the code using the aforementioned methods, running "go vet" will no longer display the mentioned warning.

Alright, the problem in the previous example was relatively easy to spot. Next, let's explore a more subtle code pattern where this pitfall is easy to overlook.

For loop + Address Operator &

Interestingly, even without closure functions, one can encounter similar issues. For instance: Reference

package main

import "fmt"

func main() {
    var out []*int
    for i := 0; i < 3; i++ {
        out = append(out, &i)
    }
    fmt.Println("Values:", *out[0], *out[1], *out[2])
    fmt.Println("Addresses:", out[0], out[1], out[2])
}

/*
Values: 3 3 3
Addresses: 0xc0000120e8 0xc0000120e8 0xc0000120e
*/

Huh, there's no closure function this time, yet the problem persists? Let's give "go vet" a try! go vet main.go

Nothing came up, huh? The results clearly aren't as expected, but "go vet" seems to think everything's fine! (Come on, go vet!)

In reality, the "i" in the for loop for each iteration is pointing to the same shared variable. To make matters worse, we're taking the address of &i and appending it to the "out" pointer slice. By the time the last iteration happens, "i" truly points to the value 3. Since they all share the same memory address, that's why the output is consistently 3.

Let's take a look at the publicly documented issue at "publicly documented issue at Lets Encrypt" and discuss a segment of the code inside.

// authz2ModelMapToPB converts a mapping of domain name to authz2Models into a
// protobuf authorizations map
func authz2ModelMapToPB(m map[string]authz2Model) (*sapb.Authorizations, error) {
    resp := &sapb.Authorizations{}
    for k, v := range m {
        // Make a copy of k because it will be reassigned with each loop.
        kCopy := k
        authzPB, err := modelToAuthzPB(&v)
        if err != nil {
            return nil, err
        }
        resp.Authz = append(resp.Authz, &sapb.Authorizations_MapElement{
            Domain: &kCopy,
            Authz: authzPB,
        })
    }
    return resp, nil
}

In line 7, a copy of "k" is made. This is because, if it wasn't copied here, the "k" at this moment would also be a shared variable. Using &k in line 13 would result in the aforementioned unexpected error.

Go 1.22 is finally addressing this common pitfall, as mentioned in "Fixing For Loops in Go 1.22".

But 1.22 hasn't been officially released yet, right? No worries, as mentioned in the article, 1.21 provides a preview of this new feature. All you have to do is enable it with the GOEXPERIMENT=loopvar flag.

GOEXPERIMENT=loopvar  go run main.go

/*
Values: 0 1 2
Addresses: 0xc0000120e8 0xc000012110 0xc000012118
*/

Perfect!

Summary

To sum up, In versions prior to Go 1.21, whenever a for loop takes the address of an iterating variable or when combining a for loop with a closure, it's crucial for code reviews to alert each other. Leveraging tools like "go vet" during Continuous Integration (CI) or using Git hooks like "husky" can help in detecting these basic errors. However, I believe Go 1.22 is a version well worth upgrading to, given how often this pitfall is encountered.