10 ms·

Go’s race detector has a mutex blind spot

79 points by GarethX 1y ago

TheDong 1y ago
You're using Go's race detector wrong if you expect it to actually catch all races. It doesn't, it can't, it's a best effort thing. The right way to use the go race detector is: 1. Only turn it on in testing. It's too slow to run in prod to be worth it, so only in testing. If your testing does not cover a use-case, tough luck, you won't catch the race until it breaks prod. 2. Have a nightly job that runs unit and integ tests, built with -race, and without caching, and if any races show up there, save the trace and hunt for them. It only works probabilistically for almost all significant real-world code, so you have to keep running it periodically. 3. Accept that you'll have, for any decently sized go project, a chunk of mysterious data-races. The upstream go project has em, most of google's go code has em, you will to. Run your code under a process manager to restart it when it crashes. If your code runs on user's devices, gaslight your users into thinking their ram or processor might be faulty so you don't have to debug races. 4. Rewrite your code in rust, and get something better than the go race detector every time you compile. The most important of those is 3. If you don't do anything else, do 3 (i.e. run your go code under systemd or k8s with 'restart=always').
- ViewTrick1002 1y ago
  The data race patterns in Go article from Uber is always a scary read. https://www.uber.com/blog/data-race-patterns-in-go/ https://www.uber.com/blog/data-race-patterns-in-go/
  - stouset 1y ago
    This is more fuel for my thesis that every single feature of golang was considered in isolation, and zero thought was put into how any of them would work together. I’m not sure how else you can explain perfectly idiomatic code (a loop, or a reused err variable, or a closure) causing a program to fall on its face simply by dropping in go’s namesake feature, whose entire purpose was supposed to be that you could simply drop it in. To actually use `go` you have to do minor contortions like always remembering to copy your loop variables, make new error variables, and also not accidentally capture any external variables in a closure. Go doesn’t actually help you with any of this, of course. You just have to remember to do it right every single time. None of these things are hard (usually), but the fact that you have to do them at all speaks volumes about the amount of forethought that went into it. And of course doing all of those steps doesn’t save you if one of the things you tried to copy secretly contains a pointer inside of it, like absolutely everything in golang does. You didn’t know, and it wasn’t even a public member so it wasn’t in the docs. But there was a pointer somewhere deep inside the thing you copied so now you’ve got unguarded concurrent mutation of shared memory.
- klabb3 1y ago
  > Rewrite your code in rust, and get something better than the go race detector every time you compile. Congrats, rustc forced you to wrap all your types in Arc<Mutex<_>>, and you no longer have data races. As a gift, you will get logical race conditions instead, that are even more difficult to detect, while being equally difficult to reproduce reliably in unit tests and patch. Don’t get me wrong, Rust has done a ton for safety and pushed other languages to do better. I love probably 50% of Rust. But Rust doesn’t protect against logical races, lovelocks, deadlocks, and so on. To write concurrent programs that have the same standards of testable, composable, expressive etc as we are expecting with sequential programs is really really difficult. Either we need new languages, frameworks or (best case) design- and architectural patterns that are easy to apply. As far as I’m concerned large scale general purpose concurrent software development is an unsolved problem.
  - catigula 1y ago
    If it's solved the solution has been discarded at some point by other developers for being too cumbersome, too much effort, and therefore in violation of some sacred principle of their job needing to be effortless.
  - ViewTrick1002 1y ago
    A well formed Go program would have the same logical race conditions to manage as well. The Arc is only needed when you truly need to mutably share data. Rust like Go has the full suite of different channels and what other patterns to share data.
    - jason_oster 1y ago
      Small correction: The Arc is for sharing across threads, the Mutex is for mutation. But you are generally correct that they can be used independently.
      - ViewTrick1002 1y ago
        Of course. But if you’re using a channel then it hides the inner constructs. Comparing writing a web service in Go and rust you would likely also utilize Tokio which has a wide variety of well designed sync primitives. https://docs.rs/tokio/latest/tokio/sync/index.html https://docs.rs/tokio/latest/tokio/sync/index.html
- onionisafruit 1y ago
  I configure ci to run tests with -race and that works out pretty well. I value short ci runs, so testing with -race is a sacrifice for me even if it only adds ~10 seconds typically. I like your idea of a regular job that runs without caching, but your best tip is gaslighting users. Maybe I should start prefixing error messages with “look what you made me do”.
- franticgecko3 1y ago
  > Have a nightly job that runs unit and integ tests Not enough IMHO. We run all tests on developer machines and CI with -race. Always. It's probabilistic, so every developer 'make test' and every 'git push' is coverage.
- aleksi 1y ago
  > It's too slow to run in prod to be worth it I disagree there. It is reasonable to run a few service instances with a race detector. I have a few services where _all_ instances are running with it just fine.
Xeoncross 1y ago
I'm so glad to be out of the dark ages of parallelism. Complaining about Go's race detector or exactly which types of logical races Rust can't prevent is such a breath of fresh air compared to all those other single-core languages we're paid to write with that had threading, async, or concurrency bolted-on as an afterthought. I can only hope Go and Rust continue to improve until the next language generation comes along to surpass them. I honestly can't wait, things improved so much already.
- deleted 1y ago
  [deleted]
- tialaramex 1y ago
  You know how a modern language like Rust doesn't have the unstructured control flow with features like "goto"† but only a set of structured control flow features, such as pattern matching, conditionals, loops and functions? Structured Concurrency is the same idea, but for concurrency. Instead of that code to create an appropriate number of threads, parcel out work, and so on, you just express high level goals like "Do these N pieces of work in any order" or "Do A and B, and once either is finished also do C and D" and just as the language handles the actual machine code jumps for your control flow, that would happen for concurrency too. Nothing as close to the metal as Rust has that baked in today, but it is beginning to be a thing in languages like Swift and you can find libraries which take this approach. † C's goto is de-fanged from the full blown go-to arbitrary jump in early languages, but it's still not structured control flow.
  - pkolaczk 1y ago
    Rust async streams or rayon come very close to what you describe as structured concurrency. Actually much closer than anything I saw in other mainstream languages eg Java or Go.
    - empath75 1y ago
      Rayon is about as pure an example of it as you can imagine. In a lot of cases you just need to replace iter() with par_iter() and it just works.
Jyaif 1y ago
I always run my Go code with `-race`, but I feel more comfortable writing C++ multithreaded code than Go thanks to the thread sanitizer annotations ( `__attribute__((guarded_by(guard)))` and others in the family). The annotation also help me discover patterns, like when most of the functions of a class have the same annotations, maybe it means that all the functions of the class should have the same annotations. I really wish an equivalent to those annotations came to Go.
reactordev 1y ago
if id == 1 { counter++; } Found your problem. /s In all honesty, if you “do work” using channels then all your goroutines are “thread safe” as the channel keeps things in order. Also, mutex is working as intended. As you see in your post, -race sees this, it’s good. Now have one goroutine read from a chan, get rid of the mutex, all other goroutines write to the chan, perfection.
gackletackle 1y ago
[dead]
thwarted 1y ago
The race detector has always only worked at run time, and is documented to detect concurrent memory accesses. This means the memory has to actually be accessed in order for it to see the race condition. It does not do static analysis. https://go.dev/blog/race-detector https://go.dev/blog/race-detector > Because of its design, the race detector can detect race conditions only when they are actually triggered by running code, which means it’s important to run race-enabled binaries under realistic workloads. This isn't a mutex blind spot. It is a side effect of goroutine/thread scheduling, which will obviously be based on workload and other factors. There are a bunch of other cases, that are not mutex related, that it won't see unless execution actually triggers them.

79 points by GarethX 1y ago

You're using Go's race detector wrong if you expect it to actually catch all races. It doesn't, it can't, it's a best effort thing. The right way to use the go race detector is: 1. Only turn it on in testing. It's too slow to run in prod to be worth it, so only in testing. If your testing does not cover a use-case, tough luck, you won't catch the race until it breaks prod. 2. Have a nightly job that runs unit and integ tests, built with -race, and without caching, and if any races show up there, save the trace and hunt for them. It only works probabilistically for almost all significant real-world code, so you have to keep running it periodically. 3. Accept that you'll have, for any decently sized go project, a chunk of mysterious data-races. The upstream go project has em, most of google's go code has em, you will to. Run your code under a process manager to restart it when it crashes. If your code runs on user's devices, gaslight your users into thinking their ram or processor might be faulty so you don't have to debug races. 4. Rewrite your code in rust, and get something better than the go race detector every time you compile. The most important of those is 3. If you don't do anything else, do 3 (i.e. run your go code under systemd or k8s with 'restart=always').

ViewTrick1002 1y ago
The data race patterns in Go article from Uber is always a scary read. https://www.uber.com/blog/data-race-patterns-in-go/ https://www.uber.com/blog/data-race-patterns-in-go/
- stouset 1y ago
  This is more fuel for my thesis that every single feature of golang was considered in isolation, and zero thought was put into how any of them would work together. I’m not sure how else you can explain perfectly idiomatic code (a loop, or a reused err variable, or a closure) causing a program to fall on its face simply by dropping in go’s namesake feature, whose entire purpose was supposed to be that you could simply drop it in. To actually use `go` you have to do minor contortions like always remembering to copy your loop variables, make new error variables, and also not accidentally capture any external variables in a closure. Go doesn’t actually help you with any of this, of course. You just have to remember to do it right every single time. None of these things are hard (usually), but the fact that you have to do them at all speaks volumes about the amount of forethought that went into it. And of course doing all of those steps doesn’t save you if one of the things you tried to copy secretly contains a pointer inside of it, like absolutely everything in golang does. You didn’t know, and it wasn’t even a public member so it wasn’t in the docs. But there was a pointer somewhere deep inside the thing you copied so now you’ve got unguarded concurrent mutation of shared memory.
klabb3 1y ago
> Rewrite your code in rust, and get something better than the go race detector every time you compile. Congrats, rustc forced you to wrap all your types in Arc<Mutex<_>>, and you no longer have data races. As a gift, you will get logical race conditions instead, that are even more difficult to detect, while being equally difficult to reproduce reliably in unit tests and patch. Don’t get me wrong, Rust has done a ton for safety and pushed other languages to do better. I love probably 50% of Rust. But Rust doesn’t protect against logical races, lovelocks, deadlocks, and so on. To write concurrent programs that have the same standards of testable, composable, expressive etc as we are expecting with sequential programs is really really difficult. Either we need new languages, frameworks or (best case) design- and architectural patterns that are easy to apply. As far as I’m concerned large scale general purpose concurrent software development is an unsolved problem.
- catigula 1y ago
  If it's solved the solution has been discarded at some point by other developers for being too cumbersome, too much effort, and therefore in violation of some sacred principle of their job needing to be effortless.
- ViewTrick1002 1y ago
  A well formed Go program would have the same logical race conditions to manage as well. The Arc is only needed when you truly need to mutably share data. Rust like Go has the full suite of different channels and what other patterns to share data.
  - jason_oster 1y ago
    Small correction: The Arc is for sharing across threads, the Mutex is for mutation. But you are generally correct that they can be used independently.
    - ViewTrick1002 1y ago
      Of course. But if you’re using a channel then it hides the inner constructs. Comparing writing a web service in Go and rust you would likely also utilize Tokio which has a wide variety of well designed sync primitives. https://docs.rs/tokio/latest/tokio/sync/index.html https://docs.rs/tokio/latest/tokio/sync/index.html
onionisafruit 1y ago
I configure ci to run tests with -race and that works out pretty well. I value short ci runs, so testing with -race is a sacrifice for me even if it only adds ~10 seconds typically. I like your idea of a regular job that runs without caching, but your best tip is gaslighting users. Maybe I should start prefixing error messages with “look what you made me do”.
franticgecko3 1y ago
> Have a nightly job that runs unit and integ tests Not enough IMHO. We run all tests on developer machines and CI with -race. Always. It's probabilistic, so every developer 'make test' and every 'git push' is coverage.
aleksi 1y ago
> It's too slow to run in prod to be worth it I disagree there. It is reasonable to run a few service instances with a race detector. I have a few services where _all_ instances are running with it just fine.

Xeoncross 1y ago

I'm so glad to be out of the dark ages of parallelism. Complaining about Go's race detector or exactly which types of logical races Rust can't prevent is such a breath of fresh air compared to all those other single-core languages we're paid to write with that had threading, async, or concurrency bolted-on as an afterthought. I can only hope Go and Rust continue to improve until the next language generation comes along to surpass them. I honestly can't wait, things improved so much already.

deleted 1y ago
[deleted]
tialaramex 1y ago
You know how a modern language like Rust doesn't have the unstructured control flow with features like "goto"† but only a set of structured control flow features, such as pattern matching, conditionals, loops and functions? Structured Concurrency is the same idea, but for concurrency. Instead of that code to create an appropriate number of threads, parcel out work, and so on, you just express high level goals like "Do these N pieces of work in any order" or "Do A and B, and once either is finished also do C and D" and just as the language handles the actual machine code jumps for your control flow, that would happen for concurrency too. Nothing as close to the metal as Rust has that baked in today, but it is beginning to be a thing in languages like Swift and you can find libraries which take this approach. † C's goto is de-fanged from the full blown go-to arbitrary jump in early languages, but it's still not structured control flow.
- pkolaczk 1y ago
  Rust async streams or rayon come very close to what you describe as structured concurrency. Actually much closer than anything I saw in other mainstream languages eg Java or Go.
  - empath75 1y ago
    Rayon is about as pure an example of it as you can imagine. In a lot of cases you just need to replace iter() with par_iter() and it just works.

Jyaif 1y ago

I always run my Go code with `-race`, but I feel more comfortable writing C++ multithreaded code than Go thanks to the thread sanitizer annotations ( `__attribute__((guarded_by(guard)))` and others in the family). The annotation also help me discover patterns, like when most of the functions of a class have the same annotations, maybe it means that all the functions of the class should have the same annotations. I really wish an equivalent to those annotations came to Go.

reactordev 1y ago

if id == 1 { counter++; } Found your problem. /s In all honesty, if you “do work” using channels then all your goroutines are “thread safe” as the channel keeps things in order. Also, mutex is working as intended. As you see in your post, -race sees this, it’s good. Now have one goroutine read from a chan, get rid of the mutex, all other goroutines write to the chan, perfection.

gackletackle 1y ago

[dead]

thwarted 1y ago

The race detector has always only worked at run time, and is documented to detect concurrent memory accesses. This means the memory has to actually be accessed in order for it to see the race condition. It does not do static analysis. https://go.dev/blog/race-detector https://go.dev/blog/race-detector > Because of its design, the race detector can detect race conditions only when they are actually triggered by running code, which means it’s important to run race-enabled binaries under realistic workloads. This isn't a mutex blind spot. It is a side effect of goroutine/thread scheduling, which will obviously be based on workload and other factors. There are a bunch of other cases, that are not mutex related, that it won't see unless execution actually triggers them.