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

[Steven Schveighoffer]

On Mon, 28 May 2012 18:36:13 -0400, 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.

Reading all of this thread, I think I agree with you.

Now, I'll point out a couple things:

1. Most code that is written with synchronized is *not* abused.
2. using synchronized(x) should not go away.

Here is what I'm thinking:

First, we give control of the mutex to the author of the type.  So instead  
of an implicit location, and implicit lazy construction, we make  
everything explicit:

Currently:

synchronized(x)
{
}

translates to

if(x.mutex == null)
    x.mutex = createmutex();
lock(x.mutex)
try
{
}
finally
{
    unlock(x.mutex);
}

So I propose it does this instead (entirely via lowering):

synchronized(x)
{
}

translates to:

auto l = x.__getMutex();
l.lock();
try
{
}
finally
{
    l.unlock();
}

So we have a few benefits here:

1. if typeof(x) does not define __getMutex, the above is a compiler error.
2. We can control the visibility of the mutex, because we can attribute  
private or protected to the __getMutex function.
3. We can control what kinds of objects of typeof(x) use mutexes.  e.g.  
define __getMutex() shared only.
4. We have control over the type of the lock, it can be anything and does  
not need to fit into some runtime function's interface.

Second, I propose a somewhat gradual transition.  I think on first  
release, we *still* keep the allocated word in the class instance, and  
keep the druntime function which lazily allocates the mutex.  Therefore,  
changing an object that currently *properly* can use synchronized(this)  
can be made to work:

class Synchronizable
{
    xxx __getMutex() shared { return _d_getObjectMonitor(this);}
}

We can deprecate that functionality, and the allocated space for the mutex  
later if desired.

-Steve