Data Race Patterns in Go

Sorry to say, but these hit close to home for me. A lot of the synchronization paradigms in Go are easy to misuse, but lead the author into thinking it's okay. the WaitGroup one is particularly poignant for me, since the race detector doesn't catch it.

I'll add one other data race goof: atomic.Value. Look at the implementation. Unlike pretty much every other language I've seen, atomic.Value isn't really atomic, since the concrete type can't ever change after being set. This stems from fact that interfaces are two words rather than one, and they can't be (hardware) atomically set. To fix it, Go just documents "hey, don't do that", and then panics if you do.

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.

  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
   }
  }

This is pretty cool. 50 million lines of code is quite a large corpus to work off of.

I'm surprised by some of them. For example, go vet nominally catches misuses of mutexes, so it's surprising that even a few of those slipped through. I wonder if those situations are a bit more complicated than the example.

Obviously, the ideal outcome is that static analysis can help eliminate as many issues as possible, by restricting the language, discouraging bad patterns, or giving the programmer more tools to detect bugs. gVisor, for example, has a really interesting tool called checklocks:

https://github.com/google/gvisor/tree/master/tools/checklock...

While it definitely has some caveats, ideas like these should help Go programs achieve a greater degree of robustness. Obviously, this class of error would be effectively prevented by borrow checking, but I suppose if you want programming language tradeoffs more tilted towards robustness, Rust already has a lot of that covered.

A dig against Rust I sometimes hear is "Oh, data race freedom isn't such a big deal, if you really need it, a garbage collected language like Java will give you that guarantee."

So now I'm hearing that Go, a garbage collected language, doesn't guarantee data race freedom? I guess it's garbage collected but not "managed" by a runtime or something?

Why go to all that effort to get off of C++ just to stop 30% short? These are C-like concurrency bugs, and you still have to use C-like "if not nil" error handling.

Why do people keep adopting this language? Where's the appeal?

> and contains approximately 2,100 unique Go services (and growing).

A side topic: this is really not something to be proud of. There used to be more people than quantity of work in Uber and engineers fought for credits by building bogus decomposed services, and the sheer number of services seems indicate it's still so.

Seems like Rob Pike and co may have failed

"The key point here is our programmers... They’re not capable of understanding a brilliant language... So, the language that we give them has to be easy for them to understand"

At least some of these would be caught by running your tests with race detection on? I haven't read the whole article yet but as soon as I read the loop variable one I was pretty sure I have written code with that exact bug and had it caught by tests...

https://go.dev/doc/articles/race_detector

Edit: at the _end_ of the post, they mention that this is the second of two blog posts talking about this, and in the first post they explain that they caught these by deploying the default race detector and why they haven't been running it as part of CI (tl;dr it's slower and more resource-expensive and they had a large backlog).

https://eng.uber.com/dynamic-data-race-detection-in-go-code/

My favorite example is the IP address type which is an alias for a slice of bytes (type IP []byte). Thus, it gets passed by reference instead of by value and you easily end up working on the same data even if you didn't plan to. This will just be a logical bug but there are data structures in Go which result in memory corruption and introduce the risk of (remote) code execution vulnerabilities.

Is it just me, or is Golang's concurrency a very double edged sword? My exposure to goroutines and channels was mind-blowing, but I still really struggle reading through Go code and understanding what's going on in memory. The way that Go "abstracts away" ownership feels more like it's hiding important details rather than unnecessary minutia.

Here's a simple question that's stumped me for some time: if multiple go routines are popping values out of a channel, does the channel need a mutex? Why do the "fan-out, fan-in?" examples in the "Pipelines and Cancellation" post on the Go blog not require mutex locks? Link here: https://go.dev/blog/pipelines

Stuff like that, along with the ambiguity of intializing stuff by value vs using make, the memory semantics of some of the primitives (slices, channels, etc). None of it was like "of course". If something is a reference, I'd rather the language tell me it's a reference. Maybe I'm still too new to the language.

”Our Go monorepo consists of about 50 million lines of code (and growing) and contains approximately 2,100 unique Go services (and growing).”

What the hell is that company doing?

Try to imagine an ERD or DFD of their day-to-day operations. 2,100 unique services…

> We developed a system to detect data races at Uber using a dynamic data race detection technique. This system, over a period of six months, detected about 2,000 data races in our Go code base, of which our developers already fixed ~1,100 data races.

This isn't open source, correct?

> Uber has adopted Golang (Go for short)

Uber has adopted Go (Golang for long)

Worth noting that some of these can be detected statically -- and some are detected by go vet (e.g., passing a sync.Mutex by value). I don't think it detects the wg.Add bug, but that seems relatively straightforward(†) to add a check for.

(†famous last words, I know)

I think the root cause of a lot of these data races is that Go has no way of marking variables/fields/pointers/etc. as immutable or constant. This makes it easy to lose track of shared mutable state.

It's not just data races--it's also logical races, which are near-impossible to detect or prevent without something like transactional memory.

Passing sync.Mutex and sync.WaitGroup as value is especially irritating considering that the context.Context is meant to be passed as such

Go picked the concurrency ideas of Erlang but then ignored the main safeguard that makes Erlang's concurrency fearless: Immutability.

And if you feel that Erlang's lack of type safety is an issue, then Gleam has you covered.

>We developed a system to detect data races at Uber using a dynamic data race detection technique.

By system do you mean a process or a tool that detects these?

What’s up with the totally broken syntax highlighting in this post, at least on iOS? 2100 micro services and not one of them is a valid syntax highlighter for blog posts.

Edit: oh I see it highlights red and underlines every keyword. I find that incredibly distracting, so much so I assumed their highlighter was broken, but also just realized they are screenshots.

> contains approximately 2,100 unique Go services (and growing)

How is that possible and what do they do???

Really good article, and gave me several ideas to track down some gremlins that have been bugging me in a go codebase recently.

Don't take me seriously: I heard people both saying Golang is a better C/Golang is a worse C

In the closure example does declaring a new variable and setting its value to the iterative or the thing being passed in, does that mitigate the pass by reference issue?

Is there a mistake in Figure 2? It looks like myResults is captured, but the capture is never used?

I wrote a deep analysis/reaction to this post:

https://medium.com/@scott_white/concurrency-in-go-is-not-mag...

tl;dr Go doesn't magically solve data races and blaming the language itself isn't well supported by the examples/data.

can you guys put it up as a SonarQube rules?

> 2. Slices are confusing types that create subtle and hard-to-diagnose data races

The "Slices" example is just nasty! Like, this is just damning for Go's promise of "_relatively_ easy and carefree concurrency".

Think about it for a second or two,

>> The reference to the slice was resized in the middle of an append operation from another async routine.

What exactly happens in these cases? How can I trust myself, as a fallible human being, to reason about such cases when I'm trying to efficiently roll up a list of results. :-/

Compared to every other remotely mainstream language, perhaps even C++, these are extremely subtle and sharp.. nigh, razor sharp edges. Yuck.

One big takeaway is this harsh realization: Golang guarantees are scant more than what is offered by pure, relatively naïve and unadulterated BASH shell programming. I still will use it, but with newfound fear.

As a multi-hundred-kloc-authoring-gopher: I love Go, and this article is killing me inside. Go appears extremely sloppy at the edges of the envelope and language boundaries, moreso than even I had ever realized prior to now.

Full-disclosure: I am disgusted by the company that is Uber, but I'm grateful to the talented folks who've cast a light on this cesspool region of Golang. Thank you!

p.s. inane aside: I never would've guessed that in 2022, Java would start looking more and more appealing in new ways. Until now I've been more or less "all-in" on Go for years.