synchronized (this[.classinfo]) in druntime and phobos

Alex Rønne Petersen alex at lycus.org
Wed May 30 11:37:51 PDT 2012


On 30-05-2012 20:26, Martin Nowak wrote:
> On Tue, 29 May 2012 00:36:13 +0200, Alex Rønne Petersen
> <alex at 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)

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
alex at lycus.org
http://lycus.org


More information about the Digitalmars-d mailing list