On Tue, 29 May 2012 00:36:13 +0200, Alex Rønne Petersen <a...@lycus.org>
wrote:
Hi,
I've seen several occurrences of synchronized (this) and synchronized
(this.classinfo) in both druntime and phobos by now. I propose that we
officially ban these patterns with extreme prejudice.
1) Locking on the object instance is a HORRIBLE IDEA. Anyone who happens
to use the object for locking will most likely end up with a deadlock on
their hands.
2) Locking on something as fundamental as type info means that any
arbitrary part of the application could cause a deadlock by doing the
same.
All occurences of this pattern use it to obtain a global unique nameable
mutex.
It's a little ugly but I fail to see fundamental issues.
synchronized(Stacktrace.classinfo)
synchronized(typeid(ArrayAllocLengthLock)) // uses an exclusive type as
named mutex
synchronized(TaskPool.classinfo)
synchronized(this.classinfo) // this a.k.a. Socket
Then there is std.concurrency.registryLock which is used in the exact same
ways as the occurences above. To do that it requires 6 lines of code, has
to successfully avoid 2 pitfalls and allocates the mutex eagerly.
Then there is core.thread which has a lot of issues with locking.
There is some suspicious code in object.rt_attachDisposeEvent
which synchronizes on it's argument.
I'm inclined to say that this is a very thin backing for such a rant.
There was one interesting argument by Regan Heath:
The problem is /not/ that you can lock any object.
The problem is /not/ that we have synchronized(object) {}
The problem is /not/ that we have synchronized classes/methods.
The problem /is/ that synchronized classes/methods use a mutex which is
exposed publicly, and it the same mutex as used by synchronized(object)
{}. This exposure/re-use makes deadlocks more likely to happen, and
harder to spot.
Now I do not see anyone synchronizing it's code on arbitrary objects or any
other kind of abuse that would increase the inherent possibility for
deadlocks.
