Virtual methods are thread unsafe

In my February 15th SD Times column on the future of concurrent programming models, I wrote "[A] basic rule for thread safety is 'either write objects with no fields or write objects with no virtual method calls'...." I received an email today from someone asking about why virtual method calls are unsafe. Here's an excerpt from my book:

Creating a safe multithreaded library is considerably more difficult than creating a single multithreaded application. Not only do you have to try to avoid deadlock in all the scenarios in which your library is used logically, you must never expose a virtual method that is called within a critical section. The problem is that if you declare a method as virtual and call it within a critical section (that is, a section in which you've acquired a Monitor), it is possible that the client programmer will override it in a way that creates a new thread and attempts to acquire the same Monitor. For this deadlock scenario to play out, the client must create a new thread since, as discussed on page 753, a call to acquire the Monitor within the same thread's call stack will succeed. This is a subtle-enough requirement that this object-oriented deadlock can sneak by a lot of unit tests and code reviews.

The injunction goes against all virtual calls: those marked virtual, interfaces, abstract classes, and delegate calls are all vulnerable to this object-oriented deadlocking. In the following program, a Library object executes the method Client.VirtualCall( ). This works alright for FineClient, but BadClient deadlocks:

//VirtualCritical.cs

//Never expose a virtual method inside a lock

using System;

using System.Threading;

using System.Collections;

\< ?xml:namespace prefix = o ns = "urn:schemas-microsoft-com:office:office" />

class Library {

[Client client;]{lang="FR" style="mso-ansi-language: FR"}

[  public Client MyClient{]{lang="FR" style="mso-ansi-language: FR"}

[    ]{lang="FR" style="mso-ansi-language: FR"}set { client = value;}

}

Thread t;

public void Run(){

ThreadStart ts = new ThreadStart(ThreadCaller);

t = new Thread(ts);

t.Name = "Library thread";

t.Start();

}

public void Stop(){

t.Abort();

t.Join();

}

void ThreadCaller(){

while (true) {

Console.WriteLine(Thread.CurrentThread.Name +

" asking for lock");

lock(this){

Console.WriteLine(Thread.CurrentThread.Name +

" acquired lock");

client.VirtualCall();

Thread.Sleep(1000);

}

Console.WriteLine(Thread.CurrentThread.Name +

" released lock");

}

}

}

abstract class Client{

public abstract void VirtualCall();

Library l;

public Library Library{

set { l = value;}

}

protected bool callDone = false;

public void LockAndTalk(){

callDone = false;

while (callDone == false) {

Console.WriteLine(this.GetType() +

" asking for lock");

lock(l){

Console.WriteLine(Thread.CurrentThread.Name +

" acquired lock");

Console.WriteLine("Virtual call executed");

Thread.Sleep(1000);

callDone = true;

}

}

Console.WriteLine(Thread.CurrentThread.Name +

" released lock");

}

}

class FineClient: Client {

public override void VirtualCall(){

LockAndTalk();

}

}

class BadClient: Client {

public override void VirtualCall(){

ThreadedLockAndTalk();

}

public void ThreadedLockAndTalk(){

ThreadStart ts = new ThreadStart(LockAndTalk);

Thread t = new Thread(ts);

t.Name = "BadClient";

t.IsBackground = true;

t.Start();

while (callDone == false) {

Thread.Sleep(1000);

}

}

}

class TestingClass {

Library l;

TestingClass(Client c){

Console.WriteLine("Testing " + c.GetType());

l = new Library();

l.MyClient = c;

c.Library = l;

l.Run();

Thread.Sleep(10000);

Console.WriteLine("Ending test now...");

l.Stop();

}

public static void Main(){

new TestingClass(new FineClient());

new TestingClass(new BadClient());

}

}

The Library class contains a reference to a Client object, whose VirtualCall( ) method is called within a lock block inside of Library.ThreadCaller( ). The two methods Library.Run( ) and Library.Stop( ) use previously discussed techniques to begin and end the ThreadCaller( ) loop.

In addition to the abstract method VirtualCall( ), the abstract class Client specifies a method called LockAndTalk( ), which acquires a lock on the Library object, outputs something to the screen, waits a second, and then releases the lock. (This violates our preference to lock(this), but it's the easiest code to demonstrate the danger of virtual method calls.)

FineClient just calls LockAndTalk( ). When LockAndTalk( ) is called in FineClient, it is being executed in the same thread that executed Library.ThreadCaller( ) and that owns the Library monitor. Therefore, FineClient( ) works just fine.

BadClient( ) implements VirtualCall( ) in a way that the Library author did not anticipate: it starts a new Thread whose ThreadStart( ) delegate, a method called ThreadedLockAndTalk( ), calls LockAndTalk( ) from within a new thread. Notice that there is no explicit attempt on the part of the BadClient programmer to lock anything; the BadClient programmer is not doing anything obviously prone to failure. However, when ThreadedLockAndTalk( ) calls LockAndTalk( ) and that method attempts to acquire the lock on the Library object, the lock attempt is being executed from a different thread than the original thread in Library.ThreadCaller( ), which of course already has the lock on the Library object. The result is that although the BadClient programmer has done nothing obviously wrong, the Library deadlocks.

Writing a multithreaded library that is reentrant, that is, can be safely invoked from multiple threads, concurrently, and in a nested manner, is difficult enough at the best of times, but it is much harder if your library makes a virtual method call while holding onto a lock. Critical sections must be as controlled as possible; a virtual method call cedes that control and makes disaster all too likely.

>