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

[Alex Rønne Petersen]

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)
> 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.

Also, I don't think calling this a rant is fair. I gave specific reasons 
for why the patterns I mentioned are bad in _libraries_ of all things 
(notice that I was talking about druntime and phobos!).

>
>
> 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.


-- 
Alex Rønne Petersen
alex at lycus.org
http://lycus.org