On 30-05-2012 20:26, Martin Nowak wrote:
On Tue, 29 May 2012 00:36:13 +0200, Alex Rønne Petersen
<[email protected]> 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)
And what happens if I synchronize on TaskPool.classinfo in my code? The
point here is that deadlocks *can* happen. If we went by the logic that
"it's not likely to go wrong, so let's not care", why do we have slices?
scope statements? exception handling?
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.
What issues specifically...?
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.
As I pointed out in another post in this thread, I've seen the pattern
used several times by people asking questions on media such as IRC. IRC
has much higher traffic than e.g. D.learn.
--
Alex Rønne Petersen
[email protected]
http://lycus.org
