Skip to content

Instantly share code, notes, and snippets.

@lavalamp
Last active October 4, 2024 14:35
Show Gist options
  • Save lavalamp/4bd23295a9f32706a48f to your computer and use it in GitHub Desktop.
Save lavalamp/4bd23295a9f32706a48f to your computer and use it in GitHub Desktop.
Golang landmines

There are three easy to make mistakes in go. I present them here in the way they are often found in the wild, not in the way that is easiest to understand.

All three of these mistakes have been made in Kubernetes code, getting past code review at least once each that I know of.

  1. Loop variables are scoped outside the loop.

What do these lines do? Make predictions and then scroll down.

func print(pi *int) { fmt.Println(*pi) }

for i := 0; i < 10; i++ {
  defer fmt.Println(i)
  defer func(){ fmt.Println(i) }()
  defer func(i int){ fmt.Println(i) }(i)
  defer print(&i)
  go fmt.Println(i)
  go func(){ fmt.Println(i) }()
}

Answers:

func print(pi *int) { fmt.Println(*pi) }

for i := 0; i < 10; i++ {
  defer fmt.Println(i) // OK; prints 9 ... 0
  defer func(){ fmt.Println(i) }() // WRONG; prints "10" 10 times
  defer func(i int){ fmt.Println(i) }(i) // OK
  defer print(&i) // WRONG; prints "10" 10 times
  go fmt.Println(i) // OK; prints 0 ... 9 in unpredictable order
  go func(){ fmt.Println(i) }() // WRONG; totally unpredictable.
}

for key, value := range myMap {
  // Same for key & value as i!
}

Everyone expects these values to be scoped inside the loop, but go reuses the same memory location for every iteration. This means that you must never let the address of key, value, or i above escape the loop. An anonymous func() { /* do something with i */ } (a "closure") is a subtle way of taking a variable's address

  1. Nil interface is not the same as having a nil pointer in the interface.
type Cat interface {
  Meow()
}

type Tabby struct {}
func (*Tabby) Meow() { fmt.Println("meow") }

func GetACat() Cat {
  var myTabby *Tabby = nil
  // Oops, we forgot to set myTabby to a real value
  return myTabby
}

func TestGetACat(t *testing.T) {
  if GetACat() == nil {
    t.Errorf("Forgot to return a real cat!")
  }
}

Guess what? The above test will NOT detect the nil pointer! This is because the interface serves as a pointer, so GetACat effectively returns a pointer to a nil pointer. Never do the thing that GetACat does, and you (and your coworkers) will be much happier. It is very easy to do this with error values. http://golang.org/doc/faq#nil_error

  1. Shadowing considered harmful to your sanity.
var ErrDidNotWork = errors.New("did not work")

func DoTheThing(reallyDoIt bool) (err error) {
  if reallyDoIt {
    result, err := tryTheThing()
    if err != nil || result != "it worked" {
      err = ErrDidNotWork
    }
  }
  return err
}

The above function will always return a nil error, because the inner err variable "shadows" the function scope variable. Adding a var result string and not using := would fix this.

@helpmelearn
Copy link

@gholt:
That is interesting. Could you please explain (or share any link that explains) why we get the output we do with your code snippet? A go rookie here trying to learn. Thank you!

@theclapp
Copy link

For posterity, here's that code in the Playground: http://play.golang.org/p/51kKuyjox6

@helpmelearn: It's because t.PrintID() is equivalent to

f := (*T).PrintID
f(t)

or, more compactly

(*T).PrintID(t)

which is similar to the first case in the original post, "defer fmt.Println(i) // OK; prints 9 ... 0". (Here's the new code in the Playground.)

In other words, a method call is equivalent to a regular function call with the receiver passed as an argument. Since the argument is evaluated at the time the defer statement is invoked, you get different values of t each time.

See Method expressions and Defer statements in the spec.

@justicezyx
Copy link

The indentation is off, which causes the numbering to break. Can you fix that?

@bobappleyard
Copy link

How to leak memory:

ch := make(chan int)
go func() {
    for {
        ch <- 0
    }
}()
for x := range ch {
     break
}

@ogier
Copy link

ogier commented Sep 22, 2016

append() might not do what you think it does.

empty := make([]int, 0, 1)
a := append(empty, 100)
b := append(empty, 200)
fmt.Printf("a = %v, b = %v\n", a, b)

Prints a = [200], b = [200]. https://play.golang.org/p/5UJKtPu8uZ

@sbogacz
Copy link

sbogacz commented Sep 22, 2016

@gholt In your example, I think the issue is just having the pointer receiver on the PrintID() method, right?

@mimoo
Copy link

mimoo commented Mar 30, 2017

shadowing in section 3 still seems to work, whereas this says that it was fixed. Isn't it weird?

I see two other issues that might come up:

  1. using PutVarint when you want to use PutUint (you will get weird results)
  2. using a 0 modulus in a math function like big.Exp (you will get an infinite loop)

@bolianyin
Copy link

I think the following case in the original post could use a better comment on the result
"go func(){ fmt.Println(i) }() // WRONG; totally unpredictable."
I think the result should be"
// WRONG; print 10 numbers, each one can be anything from 0-10. Most likely, always 10 if the go routines run after the for loop. Does that sounds right?

@pixlcrashr
Copy link

pixlcrashr commented Jul 18, 2018

In addition to the 3rd example:

You even can shadow variables without using an if statement. It is possible to define scopes "on-the-fly":

var ErrDidNotWork = errors.New("did not work")

func DoTheThing() (err error) {
    {
        result, err := tryTheThing()
        if err != nil || result != "it worked" {
            err = ErrDidNotWork
        }
    }
    return err
}

@LukeSparkLayer
Copy link

LukeSparkLayer commented Feb 7, 2024

I believe some of the first example's cases may become more intuitive with the 1.22 release, https://tip.golang.org/blog/loopvar-preview, https://go.dev/blog/go1.22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment