Does function1 and function2 use my serial port class in thread safe way?

Question

I have a wrapper class around serial port which looks something like this:

static class HASPCLass
{
  private static SerialPort m_port;
  private static bool m_initialized;
  private static int m_baudRate;
  static readonly object _syncObject = new object(); 
  public DoInitialization(int baudRate /*also could be other params*/)
  {
    lock(_syncObject)
    {
       if (!m_initialized)
       {
         Initialize(baudRate);
       }
    }
  }

  private Initialize(int baudrate /*also could have other params*/)
  {
     m_port.open(..);
     m_baudRate = baudRate;
     m_initialized = true;
  }

  private Uninitialize()
  {
     m_port.close();
     m_initialized = false;
  }



  public void Read(byte[] buff) 
  {
    lock(_syncObject)
    {
      //Other custom read stuff
      m_port.Read(buff);
    }
  }

  public void Write(byte [] buff) 
  {
    lock(_syncObject)
    {
      //Other write related code
      m_port.Write(buff);
    }
  }

  public void Close() 
  {
    lock(_syncObject)
    {
       if (m_initialized)
       {
         Uninitialize();
       }
    }
  }
}

I tried making this class thread safe. Someone initializes it - read and writes maybe used from other threads - and in the end calls Close.

Now Imagine I have two additional static methods from other class which do something like this:

public static void function1()
{
 HASPClass.Read(...);

 // Some other code

 HASPClass.Write(...);
}

public static void function2()
{
 HASPClass.Read(...);

 // Some other code

 HASPClass.Write(...);
}

For overall thread safety I also enclosed these functions in locks:

public static void function1()
{
   lock(otherlock1)
   {
     HASPClass.Read(...);

     // Some other code

     HASPClass.Write(...); 
    }
}

public static void function2()
{
   lock(otherlock1)
   {
      HASPClass.Read(...);

      // Some other code

      HASPClass.Write(...); 
    }
}

Because order in which read and writes are called might be relavant for the HASP. My question is: is now my final approach (of using function1 and function2) correct/thread safe?

Answer 1

Since you kind of use a singleton you are fine without additional locks as long as the functions do not use resources that have to be locked in // Some other code. The class itself is thread safe because it locks all uses of the variables with the same lock. This is as tight as it gets. But make sure to not introduce dead locks in the code that lies behind the comments.

In general you should make sure no one closes your object before all threads are done with it.

Besides this code example is more or less inconsistent. You don't declare it static and write no return types and all.

Edit: From the higher persepctive of the need to give commands in a special order I correct the statement and say yes you need to lock it.

But beware of dead locks. A more explicit way how this can go wrong (though I don't see it happening in your example code):

There are 2 threads that can hold the lock. Your device will always send you 1 except if you transmit 2 to it then it will send you 2.

Thread 1 is trying to first read a 1 and after that a 2 from the device without releasing the lock.

Now suppose somehow the actions taken after receiving 1 start Thread 2 which wants to transmit 2 to the device. But it can not because Thread 1 is still waiting but it will wait forever because Thread 2 can not transmit. The most often case for this is GUI events used with invoke (which leads to an other thread executing code).

Answer 2

Imagine I have two additional static methods from other class ... To ensure thread safety do I have to put additional locks ... ?

No. A lock does not care about the calling method or the stack trace - it only concerns the current thread. Since you already put locks in the critical sections, there is no point in putting higher level locks in your case.

Answer 3

You don't want a thread-safe class, you want a message queue.

By the comments I see your concern is if read/writes are mixed, you write from one thread and other issues a read before the writer thread reads the response.

In that scenario the best you can do is to create a queue of operations, when a write must read then you add a Read and Write operation in only one call, in this way the sequence will be warranted to follow the correct order, and in this way you only need to lock the queue.

Something like this:

Queue:

    public class SerialQueue
    {

        SerialPort sp;

        ManualResetEvent processQueue = new ManualResetEvent(false);
        Queue<QueueCommand> queue = new Queue<QueueCommand>();

        public event EventHandler<ReadEventArgs> ReadSuccess;
        public event EventHandler<IdEventArgs> WriteSuccess;

        public SerialQueue()
        {
            ThreadPool.QueueUserWorkItem(ProcessQueueThread);
            sp = new SerialPort(); //Initialize it according to your needs.
            sp.Open();

        }

        void ProcessQueueThread(object state)
        {

            while (true)
            {

                processQueue.WaitOne();

                QueueCommand cmd;

                while(true)
                {
                    lock (queue)
                    {
                        if (queue.Count > 0)
                            cmd = queue.Dequeue();
                        else
                        {
                            processQueue.Reset();
                            break;
                        }
                    }

                    if (cmd.Operation == SerialOperation.Write || cmd.Operation == SerialOperation.WriteRead)
                    {
                        sp.Write(cmd.BytesToWrite, 0, cmd.BytesToWrite.Length);
                        if (WriteSuccess != null)
                            WriteSuccess(this, new IdEventArgs { Id = cmd.Id });
                    }

                    if(cmd.Operation == SerialOperation.Read || cmd.Operation == SerialOperation.WriteRead)
                    {
                        byte[] buffer = new byte[cmd.BytesToRead];
                        sp.Read(buffer, 0, buffer.Length);

                        if (ReadSuccess != null)
                            ReadSuccess(this, new ReadEventArgs { Id = cmd.Id, Data = buffer });
                    }

                }
            }

        }

        public void EnqueueCommand(QueueCommand Command)
        {
            lock(queue)
            {
                queue.Enqueue(Command);
                processQueue.Set();
            }
        }
    }

QueueCommand:

    public class QueueCommand
    {

        public QueueCommand()
        {
            Id = Guid.NewGuid();
        }

        public Guid Id { get; set; }
        public SerialOperation Operation { get; set; }
        public int BytesToRead { get; set; }
        public byte[] BytesToWrite { get; set; }
    }

Enums:

    public enum SerialOperation
    { 

        Read,
        Write,
        WriteRead

    }

Event arguments:

    public class IdEventArgs : EventArgs
    {

        public Guid Id { get; set; }

    }

    public class ReadEventArgs : IdEventArgs
    {


        public byte[] Data{ get; set; }

    }

To use the queue you instantiate it and hook to the WriteSucces and ReadSucces.

 SerialQueue queue = new SerialQueue();
 queue.ReadSuccess += (o, args) =>  { /*Do whatever you need to do with the read data*/ };
 queue.WriteSuccess += (o, args) =>  { /*Do whatever you need to do after the write */ };

Note that each QueueCommand has a property named Id which is a unique Guid, it allows you to track when the commands are executed.

Now, when you want to perform a read you do:

QueueCommand cmd = new QueueCommand { Operation = SerialOperation.Read, BytesToRead = 1024 };
queue.Enqueue(cmd);

In this moment the queue will add the command and set the reset event, when the reset event is set the thread processing the commands will continue it's execution (if wasn't already executing) and process all the possible commands in the queue.

For a write you will do:

QueueCommand cmd = new QueueCommand { Operation = SerialOperation.Write, BytesToWrite = new byte[]{ 1, 10, 40 } };

And for a write followed by a read you will do:

QueueCommand cmd = new QueueCommand { Operation = SerialOperation.WriteRead, BytesToWrite = new byte[]{ 1, 10, 40 }, BytesToRead = 230 };

I have been working with serial ports for years in multi threaded environments and this is the only way to ensure sequentiallity between sent commands and received responses, else you will mix responses from different commands.

Remember this is just a base implementation, you need to add error handling and customize it to your needs.

Answer 4

The thread safety of a method has nothing to deal with serial port operations (see this interesting discussion What Makes a Method Thread-safe? What are the rules?).

At the end, I think that your lock(_syncObject) in your first class is not necessary (but I don't know the rest of your code!), if you call the methods in the way you did, because the Read() and Write() calls are enclosed in a sync-lock to the same object (I'm supposing that your lock object is declared like private static readonly object otherlock1 = new object();).

In my opinion, if you only call function1 and function2 in the rest of your code, your approach is definitely thread-safe (supposed that your // Some other code don't spawn another thread that can make some thread-unsafe operations on the same variables on which function1 and function2 are working...).

Talking about the serial port protocol, what does it happen if your // Some other code fails for some reason? For example a computation error between your HASPClass.Read(...) and HASPClass.Write(...). This might not affect the thread-safety it-self, but damage the sequence of the read-write operations (but only you can know the details on that).

Answer 5

First of all, using singletons in such manner is a bad practice. You should consider using something like this.

public sealed class SerialPortExt
{
    private readonly SerialPort _serialPort;
    private readonly object _serialPortLock = new object();

    public SerialPortExt(SerialPort serialPort)
    {
        _serialPort = serialPort;
    }

    public void DoSomething()
    {
    }

    public IDisposable Lock()
    {
        return new DisposableLock(_serialPortLock);
    }
}

Where DisposableLock looks like this.

public sealed class DisposableLock : IDisposable
{
    private readonly object _lock;

    public DisposableLock(object @lock)
    {
        _lock = @lock;
        Monitor.Enter(_lock);
    }

    #region Implementation of IDisposable

    public void Dispose()
    {
        Monitor.Exit(_lock);
    }

    #endregion
}

Then you can work with your instance in the following way.

class Program
{
    static void Main()
    {
        var serialPortExt = new SerialPortExt(new SerialPort());
        var tasks =
            new[]
                {
                    Task.Run(() => DoSomething(serialPortExt)),
                    Task.Run(() => DoSomething(serialPortExt))
                };
        Task.WaitAll(tasks);
    }

    public static void DoSomething(SerialPortExt serialPortExt)
    {
        using (serialPortExt.Lock())
        {
            serialPortExt.DoSomething();
            Thread.Sleep(TimeSpan.FromSeconds(5));
        }
    }
}

Answer 6

Since I cannot try out your code and it wouldn't compile I would just advice that you make your wrapper into a singleton and perform the locking from there.

Here is an example of your sample code converted to a singleton class based on MSDN Implementing Singleton in C#:

public class HASPCLass
{
    private static SerialPort m_port;
    private static bool m_initialized;
    private static int m_baudRate;
    static readonly object _syncObject = new object();

    private static HASPCLass _instance;

    public static HASPCLass Instance
    {
        get 
        {
            if(_instance == null)
            {
                lock(_syncObject)
                {
                    if (_instance == null)
                    {
                        _instance = new HASPCLass();
                    }
                }
            }

            return _instance;
        }
    }

    public void DoInitialization(int baudRate /*also could be other params*/)
    {
        if (!m_initialized)
        {
            Initialize(baudRate);
        }
    }

    private void Initialize(int baudrate /*also could have other params*/)
    {
        m_port.Open();
        m_baudRate = baudrate;
        m_initialized = true;
    }

    private void Uninitialize()
    {
        m_port.Close();
        m_initialized = false;
    }



    public void Read(byte[] buff)
    {
        m_port.Read(buff, 0, buff.Length);
    }

    public void Write(byte[] buff)
    {
        m_port.Write(buff, 0, buff.Length);
    }

    public void Close()
    {
        if (m_initialized)
        {
            Uninitialize();
        }
    }
}

Notice that locking is only applied on the instance of HASPCLass.

if(_instance == null)

This check is added because when multiple threads try to access the singleton instance it will be null. In this case that is the time where it should wait and check if it is currently locked. These modifications has already made your HASPCLass thread safe! Now consider adding more functions such as for setting the port name and other properties as needed.

Answer 7

Generally, in this case of situation, you have to use a Mutex(). A mutex permits mutual exclusion to shared resources.