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

[Michel Fortin]

On 2012-05-29 23:22:38 +0000, Alex Rønne Petersen <alex at lycus.org> said:

> Possibly. I haven't thought that through, but I think that that could 
> work. In fact, it would probably be a good way to ensure safety 
> statically, since a synchronized class tells its user "I take care of 
> synchronization".

More or less.

My biggest gripe about synchronized functions is that calling other 
functions within them is prone to create deadlocks. Can you see the 
deadlock in this example?

	synchronized class Node
	{
		private Node[] peers;

		void addPeer(Node peer)    // implicitly synchronized
		{
			peers ~= peer;
		}

		void poke()    // implicitly synchronized
		{
			writeln("blip!");
		}

		void pokeNext()    // implicitly synchronized
		{
			if (!peers.empty)
				peers.front.poke();
		}
	}

	shared Node node1;
	shared Node node2;

	void pokeThread(Node node)
	{
		node.pokeNext();
	}

	void main()
	{
		// setup
		node1 = new Node;
		node2 = new Node;
		node1.addPeer(node2);
		node2.addPeer(node1);

		// start two threads
		spawn(&pokeThread, node1);
		spawn(&pokeThread, node2);
	}

The correct way to write the pokeNext function would be this, assuming 
you could remove the implicit synchronization:

		void pokeNext()     // not implicitly synchronized
		{
			Node next;
			synchronized (this)
			{
				if (!peers.empty)
					next = peers.front;
			}
			if (next)
				next.poke();
		}

Here you only synchronize access to the variable, which cannot deadlock 
in any way. Keeping the lock while calling other functions is dangerous 
(other functions can take their own lock and thus deadlock) and should 
be avoided as long as you hold a lock. The easiest way to avoid 
deadlocks is to never take two locks at the same time. The problem is 
that implicit synchronized blocks makes it too easy to take many locks 
at the same time without even noticing, which is deadlock prone.

I think that is a reason enough not to use synchronized classes in the 
general case. They're fine as long as your member functions (which are 
implicitly synchronized) only access what is contained in the class, 
but you must be sure not to call another function that takes another 
lock, or then you need to be very careful about what those functions do.

(Also, being forced to take locks longer than necessary because the 
whole function is always synchronized can be a performance problem, but 
that's a relatively minor issue compared to deadlocks.)

-- 
Michel Fortin
michel.fortin at michelf.com
http://michelf.com/