Why Locking On a Public Object is a Bad Idea

Question

Ok, I've used locks quite a bit, but I've never had this scenario before. I have two different classes that contain code used to modify the same MSAccess database:

public class DatabaseNinja
{
    public void UseSQLKatana
    {
        //Code to execute queries against db.TableAwesome
    }
}

public class DatabasePirate
{
    public void UseSQLCutlass
    {
        //Code to execute queries against db.TableAwesome
    }
}

This is a problem, because transactions to the database cannot be executed in parallel, and these methods (UseSQLKatana and UseSQLCutlass) are called by different threads.

In my research, I see that it is bad practice to use a public object as a lock object so how do I lock these methods so that they don't run in tandem? Is the answer simply to have these methods in the same class? (That is actually not so simple in my real code)

Answer 1

Well, first off, you could create a third class:

internal class ImplementationDetail
{
    private static readonly object lockme = new object();
    public static void DoDatabaseQuery(whatever)
    {
        lock(lockme)
             ReallyDoQuery(whatever);
    }
}

and now UseSQLKatana and UseSQLCutlass call ImplementationDetail.DoDatabaseQuery.

Second, you could decide to not worry about it, and lock an object that is visible to both types. The primary reason to avoid that is because it becomes difficult to reason about who is locking the object, and difficult to protect against hostile partially trusted code locking the object maliciously. If you don't care about either downside then you don't have to blindly follow the guideline.

Answer 2

The reason it's bad practice to lock on a public object is that you can never be sure who ELSE is locking on that object. Although unlikely, someone else someday can decide that they want to grab your lock object, and do some process that ends up calling your code, where you lock onto that same lock object, and now you have an impossible deadlock to figure out. (It's the same issue for using 'this').

A better way to do this would be to use a public Mutex object. These are much more heavyweight, but it's much easier to debug the issue.

Answer 3

Use a Mutex.
You can create mutex in main class and call Wait method at the beginning of each class (method); then set mutex so when the other method is called it gonna wait for first class to finish.
Ah, remember to release mutex exiting from those methods...

Answer 4

I see two differing questions here:

Why is it a bad idea to lock on a public object?

The idea is that locking on an object restricts access while the lock is maintained - this means none of its members can be accessed, and other sources may not be aware of the lock and attempt to utilise the instance, even trying to acquire a lock themselves, hence causing problems.

For this reason, use a dedicated object instance to lock onto.

How do I lock these methods so that they don't run in tandem?

You could consider the Mutex class; creating a 'global' mutex will allow your classes to operate on the basis of knowing the state of the lock throughout the application. Or, you could use a shared ReaderWriterLockSlim instance, but I wouldn't really recommend the cross-class sharing of it.