5 ms·

What are some examples of people using mutexes wrong? I know one gotcha is you need to maintain a consistent hierarchy. Usually the easiest way to not get snagg

by jart 2y ago

What are some examples of people using mutexes wrong? I know one gotcha is you need to maintain a consistent hierarchy. Usually the easiest way to not get snagged by that, is to have critical sections be small and pure. Java's whole MO of letting people add a synchronized keyword to an entire method was probably not the greatest idea.

cogman10 2y ago
When, how, and why. The biggest part of mutexes and how to properly use them is thinking of the consistency of the data that you are working with. Here's a really common bug (psuedocode) if (lock {data.size()} > 0) { value = lock { data.pop() } lock { foo.add(value) } } The issue here is size can change, pop can change, and foo can change in unexpected ways between each of the acquired locks. The right way to write this code is lock { if (data.size() > 0) { value = data.pop() foo.add(value) } } That ensures the data is all in a consistent state while you are mutating it. Now, what does make this tricky is someone well-meaning might have decided to push the lock down a method. Imagine, for example, you have a `Foo` where all methods operate within a mutex. This code is also (likely) incorrect. value = foo.bar() if (value.bat()) { foo.baz(value) } The problem here is exactly the same problem as above. Between `foo.bar()` and `foo.baz()` the state of foo may have changed such that running `foo.baz(value)` is now a mistake. That's why the right thing to do is likely to have a `foo.barBaz()` method that encapsulates the `if` logic to avoid inconsistency (or to add another mutex). In java, the most common manifestation (that I see) of this looks like this var map = new ConcurrentHashMap(); if (map.get(foo) == null) map.put(foo, new Value()); Because now, you have a situation where the value of `foo` in the map could be 2 or more values depending on who gets it. So, if someone is mutating `Value` concurrently you have a weird hard to track down data race. The solution to this problem in java is map.computeIfAbsent(foo, (unused)->new Value());
- hinkley 2y ago
  Composing locks is where Java usually blows up. And computeIfAbsent can end up holding the lock for too long if the function is slow.
  - foobazgt 2y ago
    Composing locks isn't a Java problem - it's a fundamental abstraction problem with locks. This is one of the reasons why you usually reach for higher level abstractions than mutexes. > And computeIfAbsent can end up holding the lock for too long if the function is slow. How is this different from any other lock-holding code written anywhere?
    - hinkley 2y ago
      I’m saying Java is exceptionally bad at this because every object is its own mutex. And you end up having to trade single core performance for multi core by deciding to speculatively calculate the object. If there’s no object to make the critical section is very small. But as the object sprouts features you start smashing face first into Amdahl.
      - foobazgt 2y ago
        > because every object is its own mutex. Not true in any practical sense. > And you end up having to trade single core performance for multi core by deciding to speculatively calculate the object. What is the alternative you suggest? If you care about having the predicate actually hold, and you also don't want to have to hold the lock while constructing the object, then you're going to end up in an optimistic-concurrency scenario where you check the predicate under lock, compute the object, and check again before swapping the value in. You may end up having to throw your work away when you discover the predicate changed. Java is no better nor worse at doing this than anything else.
        hinkley 2y ago
        > Not true in any practical sense. This is going to put a damper on any further conversation. Even with coarsening and elision every synchronized function closes a lock on the enclosing object.
        int_19h 2y ago
- another-acct 2y ago
  Might want to move foo.add() out of the lock scope (assuming foo is a thread-private resource): value = nil lock { if (data.size() > 0) { value = data.pop() } } if (value) { foo.add(value) }
- samatman 2y ago
  This is one of the areas where Zig's combination of anonymous blocks and block-based defer really pay off. To create a locked region of code is just this { mutex.lock(); defer mutex.unlock(); // Do mutex things } It's possible to get this wrong still, of course, but both the anonymous scope and the use of `defer` make it easier to get things right. Nothing can prevent poor engineering around mutex use though. I'd want a critical path for a concurrent hashmap to look like this: { shared_map.lock(); defer shared_map.unlock(); if (shared_map.getOrNull(foo) == null) { shared_map.put(foo, new_val); } } Where the SharedMap type has an internal mutex, and a way to check it, and all operations panic if no lock has been acquired. It could have `shared_map.lockAndGet(OrNull?)(...)`, so that the kind of problem pattern you're describing would stand out on the page, but it's still a one-liner to do an atomic get when that's all you need the critical path to perform. I don't think these invariants are overly onerous to uphold, but one does have to understand that they're a hard requirement.
  - int_19h 2y ago
    This doesn't seem to add anything over and above what std::mutex in C++ or a synchronized block in Java offer?
    - senderista 2y ago
      Less than C++. defer() is strictly inferior to RAII.
- jhatemyjob 2y ago
  I digress but my autistic brain couldn't help itself. Provided that it's a recursive lock you could do this instead of adding a new method `foo.BarBaz` foo.lock { value = foo.bar() // foo.lock within this method is ignored if(value.bat()) { foo.baz() // foo.lock within this method is ignored } } Also, to catch this bug early, you could assert foo is locked in `value.bat` or something. But that may or may not be feasible depending on how the codebase is structured
tombert 2y ago
Personally I've had issues with performance because of people using `synchronized` too liberally, where they end up locking a lot more code than necessary. I've also had issues with fairly typical circular-dependencies, causing deadlock, or at least pauses that aren't strictly necessary. Deadlock doesn't happen nearly as often as textbooks have led me to believe, but it can happen with sloppily written code. In regards to Java, at this point I almost never use the `synchronized` keyword anymore and instead (if I can't easily map to some kind of queuing abstraction) use the ReentrantLock object simply because of the ability to have lock acquisition time out, and also letting you opt-in to fairness if you'd like. It's not as pretty but it's more flexible and as far as I'm aware it doesn't affect performance much. For the most part, though, in Java, you can get away without (explicit) locks by simply abusing the built-in data structures. I know they're using their own synchronization techniques behind the scenes, but I trust those to be correct more than some ad-hoc stuff I'd write as an engineer.
foobazgt 2y ago
Java's take on monitors was definitely not great, and people were emulating mutexes with them even in the language's earliest days. Still there are a lot of things that can go wrong with mutexes: forgetting to unlock in the case of exceptions, priority inversion, recursive locking, deadlock, needlessly high contention, etc. Java has had an excellent concurrency runtime with abstractions that are typically a better fit than a bare mutex for over 20 years now (c.f. Doug Lea). Synchronized still exists, because of Java's excellent backwards compatibility.
foobiekr 2y ago
I've always disliked that lock cyclic dependencies is discussed as a hierarchy when what it really comes down to is a linear order of locks. The problem with lock _hierarchies_ as a concept is that a lock really should represent serialization of access to a particular pool of data, and should make no assumptions that it being held implies some other lock's domain is also held. The code that results when people do not maintain this kind of rigor is quite terrible, but hierarchies tend to steer people into thinking that way because they imply recursively taking locks. Stated differently: locks should be taken and released in a fixed order - so locks are ranked - but there should not be a model where all lower-ranked locks must be held for a given lock to be taken. The lock protects its domain and the ordering of take and release is to prevent deadlock, but there's no requirement for completeness.

by jart 2y ago

cogman10 2y ago

When, how, and why. The biggest part of mutexes and how to properly use them is thinking of the consistency of the data that you are working with. Here's a really common bug (psuedocode) if (lock {data.size()} > 0) { value = lock { data.pop() } lock { foo.add(value) } } The issue here is size can change, pop can change, and foo can change in unexpected ways between each of the acquired locks. The right way to write this code is lock { if (data.size() > 0) { value = data.pop() foo.add(value) } } That ensures the data is all in a consistent state while you are mutating it. Now, what does make this tricky is someone well-meaning might have decided to push the lock down a method. Imagine, for example, you have a `Foo` where all methods operate within a mutex. This code is also (likely) incorrect. value = foo.bar() if (value.bat()) { foo.baz(value) } The problem here is exactly the same problem as above. Between `foo.bar()` and `foo.baz()` the state of foo may have changed such that running `foo.baz(value)` is now a mistake. That's why the right thing to do is likely to have a `foo.barBaz()` method that encapsulates the `if` logic to avoid inconsistency (or to add another mutex). In java, the most common manifestation (that I see) of this looks like this var map = new ConcurrentHashMap(); if (map.get(foo) == null) map.put(foo, new Value()); Because now, you have a situation where the value of `foo` in the map could be 2 or more values depending on who gets it. So, if someone is mutating `Value` concurrently you have a weird hard to track down data race. The solution to this problem in java is map.computeIfAbsent(foo, (unused)->new Value());

hinkley 2y ago
Composing locks is where Java usually blows up. And computeIfAbsent can end up holding the lock for too long if the function is slow.
- foobazgt 2y ago
  Composing locks isn't a Java problem - it's a fundamental abstraction problem with locks. This is one of the reasons why you usually reach for higher level abstractions than mutexes. > And computeIfAbsent can end up holding the lock for too long if the function is slow. How is this different from any other lock-holding code written anywhere?
  - hinkley 2y ago
    I’m saying Java is exceptionally bad at this because every object is its own mutex. And you end up having to trade single core performance for multi core by deciding to speculatively calculate the object. If there’s no object to make the critical section is very small. But as the object sprouts features you start smashing face first into Amdahl.
    - foobazgt 2y ago
      > because every object is its own mutex. Not true in any practical sense. > And you end up having to trade single core performance for multi core by deciding to speculatively calculate the object. What is the alternative you suggest? If you care about having the predicate actually hold, and you also don't want to have to hold the lock while constructing the object, then you're going to end up in an optimistic-concurrency scenario where you check the predicate under lock, compute the object, and check again before swapping the value in. You may end up having to throw your work away when you discover the predicate changed. Java is no better nor worse at doing this than anything else.
      - hinkley 2y ago
        > Not true in any practical sense. This is going to put a damper on any further conversation. Even with coarsening and elision every synchronized function closes a lock on the enclosing object.
        int_19h 2y ago
another-acct 2y ago
Might want to move foo.add() out of the lock scope (assuming foo is a thread-private resource): value = nil lock { if (data.size() > 0) { value = data.pop() } } if (value) { foo.add(value) }
samatman 2y ago
This is one of the areas where Zig's combination of anonymous blocks and block-based defer really pay off. To create a locked region of code is just this { mutex.lock(); defer mutex.unlock(); // Do mutex things } It's possible to get this wrong still, of course, but both the anonymous scope and the use of `defer` make it easier to get things right. Nothing can prevent poor engineering around mutex use though. I'd want a critical path for a concurrent hashmap to look like this: { shared_map.lock(); defer shared_map.unlock(); if (shared_map.getOrNull(foo) == null) { shared_map.put(foo, new_val); } } Where the SharedMap type has an internal mutex, and a way to check it, and all operations panic if no lock has been acquired. It could have `shared_map.lockAndGet(OrNull?)(...)`, so that the kind of problem pattern you're describing would stand out on the page, but it's still a one-liner to do an atomic get when that's all you need the critical path to perform. I don't think these invariants are overly onerous to uphold, but one does have to understand that they're a hard requirement.
- int_19h 2y ago
  This doesn't seem to add anything over and above what std::mutex in C++ or a synchronized block in Java offer?
  - senderista 2y ago
    Less than C++. defer() is strictly inferior to RAII.
jhatemyjob 2y ago
I digress but my autistic brain couldn't help itself. Provided that it's a recursive lock you could do this instead of adding a new method `foo.BarBaz` foo.lock { value = foo.bar() // foo.lock within this method is ignored if(value.bat()) { foo.baz() // foo.lock within this method is ignored } } Also, to catch this bug early, you could assert foo is locked in `value.bat` or something. But that may or may not be feasible depending on how the codebase is structured

tombert 2y ago

Personally I've had issues with performance because of people using `synchronized` too liberally, where they end up locking a lot more code than necessary. I've also had issues with fairly typical circular-dependencies, causing deadlock, or at least pauses that aren't strictly necessary. Deadlock doesn't happen nearly as often as textbooks have led me to believe, but it can happen with sloppily written code. In regards to Java, at this point I almost never use the `synchronized` keyword anymore and instead (if I can't easily map to some kind of queuing abstraction) use the ReentrantLock object simply because of the ability to have lock acquisition time out, and also letting you opt-in to fairness if you'd like. It's not as pretty but it's more flexible and as far as I'm aware it doesn't affect performance much. For the most part, though, in Java, you can get away without (explicit) locks by simply abusing the built-in data structures. I know they're using their own synchronization techniques behind the scenes, but I trust those to be correct more than some ad-hoc stuff I'd write as an engineer.

foobazgt 2y ago

Java's take on monitors was definitely not great, and people were emulating mutexes with them even in the language's earliest days. Still there are a lot of things that can go wrong with mutexes: forgetting to unlock in the case of exceptions, priority inversion, recursive locking, deadlock, needlessly high contention, etc. Java has had an excellent concurrency runtime with abstractions that are typically a better fit than a bare mutex for over 20 years now (c.f. Doug Lea). Synchronized still exists, because of Java's excellent backwards compatibility.

foobiekr 2y ago

I've always disliked that lock cyclic dependencies is discussed as a hierarchy when what it really comes down to is a linear order of locks. The problem with lock _hierarchies_ as a concept is that a lock really should represent serialization of access to a particular pool of data, and should make no assumptions that it being held implies some other lock's domain is also held. The code that results when people do not maintain this kind of rigor is quite terrible, but hierarchies tend to steer people into thinking that way because they imply recursively taking locks. Stated differently: locks should be taken and released in a fixed order - so locks are ranked - but there should not be a model where all lower-ranked locks must be held for a given lock to be taken. The lock protects its domain and the ordering of take and release is to prevent deadlock, but there's no requirement for completeness.