Hacker News new | ask | show | jobs
by smasher164 1471 days ago
This definitely matches my experience using Go at my previous organization.

1. Closures and concurrency really don't mix well. The loop variable capture in particular is very pernicious. There's an open issue to change this behavior in the language: https://github.com/golang/go/issues/20733.

2. Yep. I've seen this problem in our codebase. I've grown to just be very deliberate with data that needs to be shared. Put it all in a struct that's passed around by its pointer.

3. This issue is caught fairly easily by the race detector. Using a sync.Map or a lock around a map is pretty easy to communicate with other Go devs.

4. This should be documented better, but the convention around structs that should not be passed around by value is to embed a noCopy field inside. https://github.com/golang/go/issues/8005#issuecomment-190753... This will get caught by go vet, since it'll treat it like a Locker.

5 & 6. Go makes it pretty easy to do ad-hoc concurrency as you see fit. This makes it possible for people to just create channels, waitgroups, and goroutines willy-nilly. It's really important to design upfront how you're gonna do an operation concurrently, especially because there aren't many guardrails. I'd suggest that many newcomers stick with x/sync.ErrGroup (which forces you to use its Go method, and can now set a cap on the # of goroutines), and use a *sync.Mutex inside a struct in 99% of cases.

7. Didn't encounter this that often, but sharing a bunch of state between (sub)tests should already be a red flag. Either there's something global that you initialized at the very beginning (like opening a connection), or that state should be scoped and passed down to that individual test, so it can't really infect everything around it.

2 comments

> 3. This issue is caught fairly easily by the race detector. Using a sync.Map or a lock around a map is pretty easy to communicate with other Go devs.

I run my Go development server with the -race flag as a default. If it affects performance I'll turn it off but that's very rare in practice. Unfortunately a lot of applications don't run tests against their HTTP endpoints (like only internal library stuff) which is bad bad bad, but the -race flag at least helps mitigate.

To anyone reading who cares:

1) Always run your tests with the -race flag!

2) Always write tests for your HTTP handling code too!

3) Run your dev server with -race for a week and see what happens.

This will hard crash your Go program and there is nothing you can do about it. You can't recover(). Go vet will not catch anything. The -race flag will!

  package main

  import "time"

  func main() {
   m := map[int]int{}
   go poop(m)
   go poop(m)
   time.Sleep(5 * time.Second)
  }

  func poop(m map[int]int) {
   for i := 0; i < 1e10; i++ {
    m[i] = i
   }
  }
The first link notes that they don’t plan to change the semantics of the range loop, but they might dis-allow referencing it

which is… uhh ok maybe?

But go is really trying to keep backwards compatibility so they won’t just change this

Both changes break BC though. I see the underlying divergence of opinion as one of signalling: changing loop semantics would silently change code behaviour, even if it should usually be for the better there’s a bit of an 1172, whereas making the loop variable non-reference-able noisily breaks code which is almost guaranteed to be incorrect.