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