What could Go wrong with a mutex | Hacker News Reader

What could Go wrong with a mutex

evilmartians.com

49 pointsfeross58 comments

I would zoom in on the actual fix: shrinking the size of the critical section.

When you use defer to release the lock, the entirety of your function becomes the critical section. If you add another bit of code that needs the same lock, your program goes boom.

I've hit this same issue a few times when I write a new, naive HTTP handler function that needs concurrent map access, then throw apachebench at it before I commit (or k6 when it's in QA/staging).

All of my deadlocks have come from deferring the release of the lock coupled with calling another bit of code that (acquires a second lock) rather than explicitly releasing the lock as soon as I no longer need its protection.

I may still defer the release of a lock in very small functions but, now when I see it in code, it immediately sets off alarm bells to look a bit closer.

I think deferring unlocks should be more openly described as an anti-pattern. I do appreciate the convenience of defer generally speaking but it has two problems that specifically hinder effective use of mutexes:

1. it allows you to roll up additional code that doesn't need to be inside the mutex, thus keeping other threads waiting longer for an unlock

2. Benchmarks have shown[1][2] that using defer is actually more expensive than not using defer and simply having the unlock sit just before your return call(s). I get defer produces more readable and safer code, however when you have a locking function, your are concerned with unlocking quickly. So defer directly causes complications here that, in my opinion, outweighs any benefits you get with regards to readability.

Sadly a lot of mutex examples specifically use defer without addressing any of the risks of doing so. such as GoByExample.com[3]

[1] https://gist.github.com/janisz/ce19a7fa94cbc99e2835a4421ccfc...

[2] https://medium.com/i0exception/runtime-overhead-of-using-def...

[3] https://gobyexample.com/mutexes

> So defer directly causes complications here that, in my opinion, outweighs any benefits you get with regards to readability.

The primary advantage of defer is not readability, it’s safety, in the presence of both panics and code evolving.

Yes, I agree and covered your point regarding safety in literally the sentence before that which you quoted :)

You didn't, really. You mentioned it in passing then completely ignored the issue without actually addressing how to handle the issue of locking in face of go's lack of panic safety, or human errors.

> You didn't, really. You mentioned it in passing

But it was still mentioned ;) The benefits of defer wasn't the point of my post so I'm obviously not going to write a paragraph covering all the safety concerns that defer addresses.

> then completely ignored the issue without actually addressing how to handle the issue of locking in face of go's lack of panic safety, or human errors.

Because that is a separate point. But I'm happy to discuss that if you wish:

Realistically you should put as little code inside the lock as possible and accept the risk that a panic might be generated inside the lock. However if you keep your code minimal and write thorough unit tests (not just testing expected behaviour but also hammering invalid values into the function too) then you can reduce the risk enough that it is lower than the risk of bad UX due to inefficient use of mutexes.

It's not a particularly great answer and relies heavily on developers writing good code and excellent tests, but this falls back to a point I made elsewhere that Go's tooling for managing goroutines is rather poor considering how much Go is credited for making "multi-threaded" code easy. So you're left with having to trust developers to write good code.

Does Go still run defers at function exit rather than scope exit? Changing that would be a big improvement, although I guess they’d have to use a different keyword to avoid breaking compatibility.

At function exit. You can of course always create an anonymous function to emulate a block like:

  func() {
    defer something()
    ...
  }()

This basically has the same behavior as having a block there, but with the defer being called at the exit of that functions body.

There are several patterns possible which resolve this problem, your third example actually shows that, as it uses a function which only does the container access in a separate function which uses defer - this guarantees that no other code is executed while the lock is held. Alternatively, you could use an anonymous function inside your function which immediately gets called and performs the locking, lookup and deferred unlocking.

As a Lisper, this situation of course screams for having a with-lock macro which allows to add an elegant, but safe primitive which mostly takes care of this. This would usually be implemented in terms of a function which performs the locking/unlocking and calls a function which executes the body. Fortunately, Go allows doing exactly that, so this pattern could be used, here an untested sketch:

  func withLock (l Lockable, f func()) {
     l.Lock()
     defer l.Unlock()
     f()
   }

  ...
  withLock(c, func() { c.Counters[]++})
  ...

Of course, this doesn't protect you doing some potentially blocking actions in the body of the function executed, but the leaner the syntax is and as the defer is guaranteed to be executed at the end of the body, the risk is greatly reduced.

That doesn't address the overhead of defer though (point 2) which can be a problem if your locks are in a well travelled path.

One might argue that if you have a hot path that depends on mutexes then perhaps you need to restructure your code to avoid sharing data (and thus the need for a mutex). And that point would be valid, generally speaking. But there's always that one edge case where it is unavoidable (or even still the cleaner and faster code).

Yes, the overhead isn't avoided. This example was thought as the "safe" choice. If you want to be "fast", you have to very carefully analyze what you are doing and then deciding accordingly. If you have code which cannot panic, you have no need to use defer and can unlock directly after the access. And the biggest gain would be avoiding acquiring and releasing the lock in the hot path in the first place.

I think the best practice is to approach a problem from the safe direction as long as performance isn't a total disaster and then try to find the spots which carefully have to be optimized relaxing some of the safeties. But as you are dealing with small hot spots, you can manually analyze the code in question. Similar to the "unsafe" package - if you use it only punctually, the safety loss is usually not a problem.

In recent versions defer is basically zero overhead if no loops are involved. It's just a function call at every function exit. Basically what you'd do yourself manually.

You could simply place `l.unlock()` underneath the function call to remove the overhead of the defer.

Exactly. Mutexes should be held for the shortest time possible (and you should avoid calling any function you don't know or the implementation details), yet RW mutexes only make sense if the critical section is large enough. Ergo RW mutexes very rarely make sense.

> RW mutexes only make sense if the critical section is large enough. Ergo RW mutexes very rarely make sense.

I disagree :) Exclusive locks have unavoidable cache-line ping-ponging on the state, which can make a contended lock very expensive. In a write-infrequently read-frequently scenario, mutexes can do clever things to avoid that, for example have core-local reader counts.

Also there's a middle-ground between "pointer swap" and "so large that that it will block the writers enough to notice" critical sections, and often readers are in that sweet spot.

Well, yes you enter the realm of asymmetric synchronization (I like seqlocks myself), but I feel these are no longer general purpose synchronisation primitives but have often application specific trade-offs. Generic RWlocks are usually not great and do not come with these optimization

Also, yes my critical sections are often closer to the pointer swap case that might color my expectations.

My expectations may be colored by having a pretty good default implementation of a shared mutex in our codebase :) (folly::SharedMutex)

I've had to resort to seqlocks + async reclamation (either RCU or hazard pointers, depending on the case) only in a handful of very very specific cases.

Thanks, I'll take a look at folly::SharedMutex!

I’ll say that using a worker pool is, IMO, a bit tricker than it sounds. The whole point of making goroutine creation cheap in the first place is that you can just spawn a ton of them, and then use blocking calls anywhere you want. If you have a pool of goroutines, then you open yourself to the condition that all the goroutines in the pool block, waiting for code to execute that can’t schedule because there are no available goroutines.

Any time you want to limit concurrency (e.g. by using a pool), my experience is that you have to think hard about the correct way to do that. This despite the fact that the underlying idea of a worker pool seems very innocent and easy to use. Read a few papers on the topic and you’ll see a variety of approaches. For example, if work blocks, do you give up the slot to other work? No easy answer!

I don't think using a worker pool is so bad, and I usually use one in preference to spawning lots of goroutines. I tend to follow this pattern https://play.golang.org/p/KtCPYaDiVFl, in which synchronization between the workers and the master relies on the language's built-in features and does not use explicit mutexes.

> For example, if work blocks, do you give up the slot to other work? No easy answer!

That's true in languages where threads are expensive, but goroutines are so cheap that you can probably avoid the problem simply by running a very large pool of workers.

People talk about the lack of generics et al as being Go's downfall but personally what I've found to be the biggest annoyance is the very thing Go boasts to be good at: writing bug/race free multi-threaded code.

Channels are vastly oversold given their actual suitability and worse still, add their own classes of race conditions (deadlocks) so you often fallback to using mutexes for those, hopefully rare, occasions you need to pass a shared stated. And once you're reliant on mutexes then you're really no better off than you were with any other language where threading is an afterthought.

The only real benefit Go brings is the ease of spinning new goroutines, but that can be a double edged sword if you don't understand how to manage them in the first place, and the green thread model making goroutines lightweight. Though even this isn't exactly unique to Go either.

I feel like this is one of Go's biggest weaknesses yet it seldom seems to get the same discussion generics and error handling (both of which are annoyances in specific edge cases but neither of which cause the same level of show stopper bugs).

Is there a language where deadlocks are not possible ?

Avoiding deadlocks is easy if you're lockfree (or otherwise non-blocking), the interesting bit is being data-race free:

https://www.ponylang.io/discover/#what-makes-pony-different

Now that's something remarkable about a language. Definitely going to check out Pony now.

> Avoiding deadlocks is easy if you're lockfree (or otherwise non-blocking)

Only if you’re also concurrency free.

If you can still have two actors waiting on one another, you’re letter-of-the-law deadlock-free, but your program is still in effect deadlocked.

> the interesting bit is being data-race free:

Data race freedom is much weaker (and easier) than deadlock freedom, or race condition freedom.

Safe rust is free of data races for instance. But still subject to deadlocks, and other race conditions.

> Only if you’re also concurrency free. If you can still have two actors waiting on one another, you’re letter-of-the-law deadlock-free, but your program is still in effect deadlocked.

I'm not sure we're using the same definitions. If you're non-blocking you have system wide progress, I'm not sure your examples of two actors mutually waiting on each other qualify.

In any case I think Pony qualifies as wait-free too, which avoids this scenario. Of course one can still have an infinite for loop, or an infinite ping-pong between actors.

> I'm not sure your examples of two actors mutually waiting on each other qualify

Of course it qualifies, that's a classical text book example of deadlock.

Isn't that a live lock?

Can anything really shield you from that? It seems nothing can, in the same way nothing can really shield you from an infinite loop.

> in the same way nothing can really shield you from an infinite loop.

Fwiw that’s possible with dependent typing, the compiler can require a proof of progress (and thus termination).

> Isn't that a live lock?

If they are blocked it is not a live lock. If they are spinning in a try_read then it would be a live lock.

> Can anything really shield you from that? It seems nothing can, in the same way nothing can really shield you from an infinite loop

Non-Turing Complete languages can be proven to terminate(they do exist, see total functional programming). Similarly I think there are programming paradigms that might be truly deadlock free but my recollection is fuzzy. IIRC writes must never block and reads must handle all blocking conditions with guarded commands. I think you can still livelock (by failing to do something when woken up for example).

> If they are blocked it is not a live lock.

If they're blocked then we are no longer talking about non-blocking, did we go full circle?

> Non-Turing Complete languages can be proven to terminate [...]

Of course, and while I appreciate the point for sake of argument, these languages have very limited applicability.

> Similarly I think there are programming paradigms that might be truly deadlock free but my recollection is fuzzy.

Look up Pony, from an earlier message in this thread :)

> Look up Pony, from an earlier message in this thread :)

after 5 minutes of googling, pony has non-blocking reads and uses promises with continuations. The CPS transform of a program that can deadlock will still deadlock. Just because you reify your wait queue does not mean you are free from deadlocks. They are just much harder to debug as you do not have a clean stackframe and process identity.

edit: to be more clear, to say that a program that uses promises and continuations can't deadlock is equivalent to saying that a C program running on linux can't deadlock because the kernel is still able to make progress. From the kernel point of view the C program looks exactly like the pony program, with continuations suspended on wait queues.

My understanding is that it doesn't use promises with continuations, what source suggests it does? The message queues are unbounded, do not block on writes, and only one can actor can read. There's no blocking nor waiting, so I'm not sure you can get into two actors waiting on each other at all. Infinite loops, sort-of live locks, and OOM are all still possible.

More in this presentation[0] (slides here [1]) and this [2].

[0] https://www.infoq.com/presentations/pony-type-system/

[1] https://qconlondon.com/london-2017/system/files/presentation...

[2] https://www.infoq.com/podcasts/sylvan-clebsch-pony-formal-ve...

From a cursory look and from my understanding of [1] and [2], promise next takes the continuation to apply to the future (elsewhere known as 'then').

[1] https://kevinhoffman.medium.com/modeling-non-blocking-intera...

[2] https://stdlib.ponylang.io/promises-Promise/

I think that's just normal code, nothing that affects the runtime, that Promise is just a pony actor and abides to the same rules, with the same causal messaging guarantees.

Let's say I'm writing a trivial proxy which simply forwards data unchanged between two sockets. This is my pseudocode:

   while true:
       x = read s1
       write s2, x
       y = read s2
       write s1, y

This can deadlock. If for example s1 stops sending data, read(s1) blocks, so the proxy won't forward any data from s2 back to s1, potentially leading to a full pipeline deadlock.

Now, the above can be expressed in purely non blocking fashion as follow :

   letrec f = async_read s1, \x ->
              async_write s2, x \->
              async_read s2, \y ->
              async_write s1, y, f

It still deadlocks exactly as the previous program.

I'm not going to learn pony for the sake of an example, but I'm 97.5% sure you can express the above in Pony, except you do not pass the continuation directly to read and writes but to the returned promise. Sure, the pony runtime will still work and technically the actor could potentially handle other messages, but that's not very useful for the end user that has to deal with a deadlocked pipeline.

So claiming that pony is deadlock-free might be true in a trivial sense, but in practice it is at the very least misleading and potentially dangerous.

edit: it is of course possible to write the program in such a way that it doesn't deadlock, the point is that if pony was truly deadlock free it would reject all programs that can deadlock.

edit2: and of course, if one were to show that the above program is in fact not expressible in pony, it would be a great result and I would want to know about!

I think you might not be able to write that program, there simply is no way to block in Pony (modulo FFI).

The scheduler wakes up actors that have messages (in their local/private queue) to process, all actors can do is to consume their queue and send (wait-free) messages to other actors if they want to.

From your example "read X" would be reading from the actor's queue, so it would not be blocking. If the message queue is empty that actor won't be scheduled, no blocking there.

That "while true" would not be writeable as such, you'd have to have two "behaviors" (in Pony terms, you can imagine it's an event handler) one that process the writing of s1 and one for the writing of s2, and "on read" they'd do the write. There'd be no "while true".

> I'm not going to learn pony for the sake of an example ... > edit2: and of course, if one were to show that the above program is in fact not expressible in pony, it would be a great result and I would want to know about!

I might have misunderstood what your example was supposed to do, but I suspect you'd find it interesting to try it out.

The async_{read,write} variant never blocks nor has a while loop, it is just a four continuations recursively tail calling each other. Yet it deadlocks (as it is a purely mechanical transformation of the synchronous variant). It still might be possible that the precise pony scoping rules for closures prevent writing that variant, but I would be surprised.

> If you can still have two actors waiting on one another

There's no way to actively wait for another actor (except for using callbacks). The only way for your situation to occur would be if one of the actors receives a message with an included callback, but decides to never call it.

> If you can still have two actors waiting on one another, you’re letter-of-the-law deadlock-free, but your program is still in effect deadlocked.

How is it letter-of-the-law deadlock-free?

None that I've used. But some are better than others for mitigating such scenarios. Go is pretty poor in that regard. I don't think it helped that the early releases were single threaded by default -- the argument being to help with developer onboarding. But really pain points like these need to be flushed out while the language was still being defined rather than hidden away.

Unlikely in a general purpose language

I expect you have to go out of your way to make it happen in erlang and actor based systems

Yes, Idris. Dependent types in an effect system allow you to prove that deadlock cannot occur, such that a potential deadlock in user code is a compile time error. Edwin Brady has been working on this for decades[1], and Idris 2[2] has finally made it a reality[3].

1. https://www.type-driven.org.uk/edwinb/papers/fi-cbc.pdf (free; 2009)

2. https://livebook.manning.com/book/type-driven-development-wi... (registration wall; 2017)

3. https://research-repository.st-andrews.ac.uk/handle/10023/23... (free; 2021)

Yes, I fully agree. The basics and primitives are very simple and easy, but actually using those to make useful packages is far harder than it might seem at first glance.

Goroutines and channels are not bad at all, but I feel there's some part missing to use them effectively without too much headaches. I've often found sync.WaitGroup and mutexes far more useful than channels.

Actually, I wrote an entire article about this, discussed here: https://news.ycombinator.com/item?id=26220693

Generics could for instance have helped implement a mutex that disallows access to the memory that needs to be synchronized without locking it first. This of course wouldn't have helped here, as the root cause was holding on to the mutex for too long.

I was under the impression that Go had first party tooling for deadlock checking, something called threadanalyzer if I recall correctly.

> Generics could for instance have helped implement a mutex that disallows access to the memory that needs to be synchronized without locking it first.

Did any language actually do that before rust?

It's also somewhat less useful without compiler-checked ownership as you can easily have the protected data escape the lock.

sync-like classes [1] have been used for a long time in C++. I'm pretty sure that Stroustrup had one in his book. Of course it is always easy to subvert this kind of protection in C++ as you can easily leak references outside of the implicit critical section.

[1] https://gcc.godbolt.org/z/3Wefz3cjK

I don't think I've ever seen anyone saying that Go is supposed to be good at writing bug/race-free multi-threaded code.

I concur. Although I perfectly understand why they were chosen, Go's concurrent synchronization primitives are not a good design choice. The developers of Go wanted to have primitives out of which better abstractions can be built while leaving freedom to the programmer, it makes sense and is consistent with Go's philosophy. In my experience, however, there is more buggy concurrent code out there than code that works correctly, and this could have been avoided by some higher level abstraction like the actor model or Ada's tasking with Rendezvous (where the latter is not so different from channels, to be fair).

It's a real problem because of 3rd party libraries. Too many 3rd party libraries produce deadlocks in unusual scenarios, and it can take almost as much time to write the library on your own than to find out whether their use of goroutines is correct. Deadlocks are really everywhere. The same with sync.atomic by the way, the use of these primitives is rarely correct and people keep using them despite the warnings in the docs. I'm not claiming to be smarter, on the contrary, the problem really is that Go's choice of concurrency primitives is only suitable for concurrency experts and most programmers (including myself) aren't.

The write priority of RW locks is a classical pitfall. Even knowing about this, I ended up falling into the exact same bug described in the article at least a couple of times.

If anyone is wondering why RW mutexes block new readers if there is a waiting writer, the reason is to prevent writer starvation: if, say, two threads continuously acquire and release read locks, there may never be a point in time in which no thread is holding a lock, and the writer is stuck waiting forever. Some mutexes can be configured to use read priority, but that is something that should only be done if it can be proven that starvation cannot happen.

This is a reminder that read locks do not absolve from being careful about what happens in the critical section; it should be made as small as possible no matter whether the lock is exclusive or shared.

Here this strikes me as an awful API though, it seems rather insane that `Schedule` would just block with no timeout or clear indication.

The goroutine-inspect tool they used looks really useful.

Does anyone know if there's a similar tool for Rust? I've had a few times in the past where I'm trying to figure out why some async application using tokio is hanging and having a similar tool would be very helpful.

There is Loom[1] (part of the Tokio project) for exhaustively testing multithreaded code. Though as far as I can tell it is designed for debugging threads, not async tasks.

[1] https://github.com/tokio-rs/loom

Indeed. Loom is about simulating what-happens-before questions for concurrency, particularly Atomics.

Loom would show you if your program, in which you believed A must happen, then B or C, and then either D or the other of B and C; actually could sometimes result in D happening, then B, then A because you screwed up.

Safe Rust promises that your program won't have Undefined Behaviour as a result of such a mistake, but its defined behaviour might still be extremely surprising to you. From Rust's point of view, "Open fuel valve, allow hydrogen gas to flood into engine room, then start fans, then try to light pilot flame" is no less "safe" than "Check fans are running, check pilot flame on, if so open fuel valve" but you probably don't agree. People can write safe Rust abstractions and this is popular in the embedded world, but ultimately program logic is somewhere a programmer's responsibility. Maybe you want to cause explosions in the engine room.

I recently researched this after speccing out which language to choose for some data heavy async apps at work - And the answer is right now, unfortunately, no.

Rust is in a bit of a weird place here since as a language it only provides async primitives, and it relies on frameworks bringing their own async runtimes - So the ball is in the Tokio teams' court to provide.

It is nice to use channels and tasks in Tokio - The stalwart Rust compiler catches pitfalls with memory leaks and deadlocks that you could fall into with Go. However, observing what's happening within the Tokio runtime is a far cry from where Go and its profiling tools are at.

https://github.com/tokio-rs/console - here's an attempt to make a TUI where you can watch everything in motion, which is about as close as you can get to an easy UI into the system.

There's one in the works: https://github.com/tokio-rs/console

One way I often debug this kind of problem is to use go's built-in pprof tooling. You can insert a profiling server into your program in four lines: https://pkg.go.dev/net/http/pprof. Then, do something like

    go tool pprof 'http://127.0.0.1:6060/debug/pprof/goroutine?debug=1'

to get a goroutine profile of your running program, and use the web command to display it in a browser as a DAG. The stuck goroutine can usually be seen pretty easily on the graph.

Discussed previously: https://news.ycombinator.com/item?id=29012675

Reply on news.ycombinator.com