Locking on a singleton class for thread safe device IO?

Question

Say I have the singleton class Singleton that can read and write to a SerialPort.

public sealed class Singleton
{
    private static readonly Lazy<Singleton> lazy =
        new Lazy<Singleton>(() => new Singleton());

    public static Singleton Instance { get { return lazy.Value; } }

    SerialPort commPort = new SerialPort();

    private Singleton()
    {
        // Setup SerialPort
    }

    public String Read()
    {
        return commPort.ReadLine();
    }

    public void Write(String cmd)
    {
        commPort.WriteLine(cmd);
    }
} 

Now lets also say that I have multiple threads that access the device at the end of the SerialPort. Some threads might only write to the SerialPort and some might write and then read from the SerialPort.

I want to make sure that while a thread is doing a read then write that it is not interrupted by another thread. Would the way to do this be to lock on the Singleton.Instance itself?

// Running on thread 1
public Boolean SetLEDStatus(int onOff)
{
    lock(Singleton.Instance)
    {
        Singleton.Instance.Write("SET LED " + onOff.ToString() + "\r\n");
        String status = Singleton.Instance.ReadLine();
        return (status.Contains("SUCCESS")) ? true : false;
    }
}

// Running on thread 2
public Boolean IsLEDOn()
{
    lock(Singleton.Instance)
    {
        Singleton.Instance.Write("GET LED\r\n");
        return (Singleton.Instance.ReadLine().Contains("ON")) ? true : false;
    }
}

In this instance, if SetLEDStatus and IsLEDOn were called very close to the same time, I want to make sure that the SerialPort is not written too twice before it is read. Does my use of locking prevent that?

Would this type of action be called "transactional IO"?

If this is indeed correct, are there any other more efficient ways to perform that same type of actions?

EDIT:

I understand why locking on the Singleton.Instance could be bad, if something were to lock on Singleton.Instance and then call a method in Singleton.Instance that also tries to lock on itself, there would be a deadlock.

I originally planned to use a private object in the singleton to lock on. But I kind of talked myself out of it because of the situation outlined below. Which, I am not sure if this is correct.

(Using the two methods (minues the locking) above running on Thread1 and Thread2)

1. Thread1 calls Write, Singleton.Instance locks
2. Thread2 calls Write, but is blocked by the lock
3. Singleton.Instance completes the Write and releases the lock
4. Thread2s call to Write executes, Singleton.Instance locks
5. Thread1 calls Read, but is blocked by the lock
6. Singleton.Instance completes the Write and releases the lock
7. Thread1s Read executes, Singleton.Instance locks
8. Thread2 calls Read, but is blocked by the lock
9. Singleton.Instance completes the Read and releases the lock
10. Thread2s Read is executed, Singleton.Instance locks
11. Singleton.Instance completes the Read and releases the lock

In this case there are two Writes to the serial port in a row which is improper. I need to be able to do a Write Read back to back for some types of communication.

Answer 1

For the lock object, I would use a private field on the class (i.e. not static) instead of the singleton instance itself, using the same reasoning on why not to lock(this) ever.

I usually use a declaration looks like this, as declaring the lock object is more readable as self-documented code.

private readonly object _LEDLock = new object();

This way, when someone else goes to look, they say "Oh, this is the lock object that guards thread access to the LED resource."

IMHO, I think the behavior in the SetLEDStatus and IsLEDOn methods (with locking) would be better encapsulated in your Singleton class as follows:

public sealed class Singleton
{
    private static readonly Lazy<Singleton> lazy =
        new Lazy<Singleton>(() => new Singleton());

    public static Singleton Instance { get { return lazy.Value; } }

    SerialPort commPort = new SerialPort();

    private readonly object _LEDLock = new object();

    private Singleton()
    {
        // Setup SerialPort
    }

    /// <summary>
    /// This goes in the singleton class, because this is the class actually doing the work.
    /// The behavior belongs in this class. Now it can be called with thread-safety from
    /// any number of threads
    /// </summary>
    public Boolean SetLEDStatus(int onOff)
    {
        lock(_LEDLock)
        {
            var cmd = "SET LED " + onOff.ToString() + "\r\n";
            commPort.WriteLine(cmd);
            string status = commPort.ReadLine();
            return (status.Contains("SUCCESS")) ? true : false;
        }
    }

    public Boolean IsLEDOn()
    {
        lock(_LEDLock)
        {
            commPort.Write("GET LED\r\n");
            var result = commPort.ReadLine().Contains("ON")) ? true : false;
            return result;
        }
    }
} 

Now, any calling thread can call these methods in a thread-safe manner.

Answer 2

You should never lock on public objects (on objects that are accesible by the consumer of your code or by yourself outside the type that instantiate them). You always should rely on locking only on private objects so that you will be sure that nobody will make unexpected locks and to arrive into a deadlock situation. Otherwise your implementation is OK, you must allow only one thread to access your port.

Since you already have a private object in your singleton wich is the SerialPort instance, redesign your class like my example below:

public sealed class Singleton
{
    private static readonly Lazy<Singleton> lazy =
        new Lazy<Singleton>(() => new Singleton());

    public static Singleton Instance { get { return lazy.Value; } }

    private SerialPort commPort = new SerialPort();


    private Singleton()
    {
        // Setup SerialPort
    }

    public String Read()
    {
        lock (commPort)
            return commPort.ReadLine();
    }

    public void Write(String cmd)
    {
        lock (commPort)
            commPort.WriteLine(cmd);
    }
} 

From the SerialPort documentation, we can deduce that write and read are NOT thread safe: http://msdn.microsoft.com/en-us/library/system.io.ports.serialport.aspx. So yes, you must sync your R/W to SerialPorts.

Answer 3

Does my use of locking prevent that?

Yes, because only one thread at a time can execute inside of these two methods. This is safe and I see no issue with that. Consider locking on a private object created with new object(). That's a little safer because it guards against certain bugs.

Would this type of action be called "transactional IO"?

That term is not well known. Whatever it means this is not "transactional IO".