5 ms·

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 th

by oogali 5y ago

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.

hnlmorg 5y ago
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/ce19a7fa94cbc99e2835a4421ccfc8d0 https://gist.github.com/janisz/ce19a7fa94cbc99e2835a4421ccfc... [2] https://medium.com/i0exception/runtime-overhead-of-using-defer-in-go-7140d5c40e32 https://medium.com/i0exception/runtime-overhead-of-using-def... [3] https://gobyexample.com/mutexes https://gobyexample.com/mutexes
- masklinn 5y ago
  > 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.
  - hnlmorg 5y ago
    Yes, I agree and covered your point regarding safety in literally the sentence before that which you quoted :)
    - masklinn 5y ago
      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.
      - hnlmorg 5y ago
        > 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.
  - iainmerrick 5y ago
    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.
    - 5y ago
- _ph_ 5y ago
  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.
  - hnlmorg 5y ago
    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).
    - _ph_ 5y ago
      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.
gpderetta 5y ago
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.
- ot 5y ago
  > 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.
  - gpderetta 5y ago
    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.
    - ot 5y ago
      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.
      - gpderetta 5y ago
        Thanks, I'll take a look at folly::SharedMutex!

by oogali 5y ago

hnlmorg 5y ago

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/ce19a7fa94cbc99e2835a4421ccfc8d0 https://gist.github.com/janisz/ce19a7fa94cbc99e2835a4421ccfc... [2] https://medium.com/i0exception/runtime-overhead-of-using-defer-in-go-7140d5c40e32 https://medium.com/i0exception/runtime-overhead-of-using-def... [3] https://gobyexample.com/mutexes https://gobyexample.com/mutexes

masklinn 5y ago
> 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.
- hnlmorg 5y ago
  Yes, I agree and covered your point regarding safety in literally the sentence before that which you quoted :)
  - masklinn 5y ago
    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.
    - hnlmorg 5y ago
      > 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.
- iainmerrick 5y ago
  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.
  - 5y ago
_ph_ 5y ago
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.
- hnlmorg 5y ago
  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).
  - _ph_ 5y ago
    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.

gpderetta 5y ago

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.

ot 5y ago
> 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.
- gpderetta 5y ago
  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.
  - ot 5y ago
    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.
    - gpderetta 5y ago
      Thanks, I'll take a look at folly::SharedMutex!