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 right way to write this code is
lock {
if (data.size() > 0) {
value = data.pop()
foo.add(value)
}
}
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)
}
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());
The solution to this problem in java is
map.computeIfAbsent(foo, (unused)->new Value());And computeIfAbsent can end up holding the lock for too long if the function is slow.
> 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?
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.
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.
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.
OpenTelemetry is a fairly recent library. Even if you ignore some test fakes (where, let's face it, who cares), it still uses it in a few places, and uses lock objects in others. I don't see much evidence of recursion going on with the former. But that's how things always start and later there's running and screaming.
I wonder where this patently bad idea of every object carrying its own publicly accessible mutex originated in the first place. Did Java introduce it, or did it also copy that from somewhere else? And what was the motivation?
.NET also has MethodImplOptions.Synchronized like Java does. However, the only place I have ever seen this attribute was on TextWriter.Synchronized implementation in CoreLib and nowhere else.
Java itself has `Lock` and `Condition`. In the end, most synchronization primitives do the same high-level actions and bound to end up having similar API.
As for `lock(this)`, much like with many other historically abused techniques that have become frowned upon - it's not bad per se if you own the type and know that it is internal and will not be observed outside of the assembly it is defined in, provided it is small enough. It's footgun-prone, but generally very few code paths will lock an arbitrary object instance at all, so most of the time it's something you see so rarely it has become "just write a comment why and move on" when using it. Of course this requires more deliberation and it's easier to default to blanket policies that ignore context. It can be difficult to get people to "use the appropriate tool" mentality.
.NET is also getting it's a separate `Lock` type, on top of all the existing synchronization primitives, to move a little further away from other legacy aspects of `lock`ing on object instances.
In practice this means that unless you can guarantee that you never, ever leak a reference anywhere, you don't know who else might be locking it. Which makes it impossible to reason about possible deadlocks. So the only sane way to manage it is to have a separate object used just for locking, which is never ever passed outside of the object that owns the lock.
And yes, this is absolutely bad design. There's no reason why every object needs a lock, for starters - for the vast majority of them, it's just unnecessary overhead (and yes, I know the monitors are lazily created, but every object header still needs space to store the reference to it). Then of course the fact that it's there means that people take the easy path and just lock objects directly instead of creating separate locks, just because it's slightly less code - and then things break. It's almost always the wrong granularity, too.
Thing is, I haven't seen this design anywhere outside of Java and .NET (which copied it from Java along with so many other bad ideas). Everybody else uses the sane and obvious approach of creating locks explicitly if and when they are needed.
Right. Synchronized is the key word here. The vast majority of code doesn't involve synchronized, and therefore the vast majority of objects don't have locks associated with them. That's quite important.
Those classes which do use synchronized were just going to create a ReentrantLock held for the duration of the call anyway, in which case it's all monitorEnter and monitorExit, regardless.
> This is going to put a damper on any further conversation.
Sadge.
Oops, I need to correct myself!
ReentrantLock doesn't depend upon monitorEnter/Exit, but rather AbstractQueuedSynchronizer and LockSupport, which ultimately delegate to Unsafe methods like park/unpark and CAS (*compareAndSet*). Don't know why I had that confused in my head.
In any case, the point holds that "synchronized" as a language feature has mostly a zero cost for code that doesn't use it. It's a red herring when discussing modern Java concurrency.
value = nil
lock {
if (data.size() > 0) {
value = data.pop()
}
}
if (value) {
foo.add(value)
} {
mutex.lock();
defer mutex.unlock();
// Do mutex things
}
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);
}
}
I don't think these invariants are overly onerous to uphold, but one does have to understand that they're a hard requirement.
foo.lock {
value = foo.bar() // foo.lock within this method is ignored
if(value.bat()) {
foo.baz() // foo.lock within this method is ignored
}
}
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.
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.
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.