Locking error in c#

Question

I am trying to do a request-response communication module in c#, using a SerialPort. The This is a very simple implementation, just to demonstrate that it kinda-works (SerialPort is not working properly (it is a USB virtual COM port), and sometimes eats a few characters, probably some windows driver bug).

However the demo does not work :-/

When Using a propertygrid on the form, which reads out properties of an object, which in turn sends a request to read a property from the remote device, something very strange happens: More than one simulteneous call to SendCommand is made at once.

I tried using a lock{} block to make the calls sequenctial, but it does not work. Even with the lock, more than one call is enters the protected area.

Can you please tell me what am I doing wrong?

My code:

    SerialPort sp;

    public byte[] SendCommand(byte[] command)
      {
          //System.Threading.Thread.Sleep(100);
          lock (sp)
          {
              Console.Out.WriteLine("ENTER");
              try
              {

                  string base64 = Convert.ToBase64String(command);

                  string request = String.Format("{0}{1}\r", target_UID, base64);

                  Console.Out.Write("Sending request... {0}", request);

                  sp.Write(request);

                  string response;

                  do
                  {
                      response = sp.ReadLine();
                  } while (response.Contains("QQ=="));

                  Console.Out.Write("Response is: {0}", response);

                  return Convert.FromBase64String(response.Substring(target_UID.Length));
              }

              catch (Exception e)
              {
                  Console.WriteLine("ERROR!");
                  throw e;
              }
              finally
              {
                  Console.Out.WriteLine("EXIT");
              }
          }

      }

The output:

ENTER
Sending request... C02UgAABAA=
Response is: cgAABAAARwAAAA==

EXIT
ENTER
Sending request... C02UgQARwA=
ENTER
Sending request... C02UgAABAA=
Response is: gQARwAAPHhtbD48bWVzc2FnZT5IZWxsbyBYWDIhPC9tZXNzYWdlPjxkZXN0aW5haXRvbj5NaXNpPC9kZXN0aW5hdGlvbj48L3htbD4=

Notice the two ENTER-s, without an EXIT between them? How is that possible?

Answer 1

You need to keep in mind what the lock keyword does, it allows only one thread to enter the lock. Problem is, you are not using any threads. All of this code runs on the UI thread, the main thread of your program.

The next detail you need to know is that the UI thread is special, it is re-entrant. The sp.ReadLine(); call is going to block the UI thread. That is illegal, the UI thread of a GUI program operates as a "single threaded apartment", enabled by the [STAThread] attribute on your program's Main() method. The contract of an STA thread forbids it from blocking, that's very likely to cause deadlock.

To follow the requirements of an STA, the CLR does something special whenever code that runs on the UI thread performs a blocking operation, like SerialPort.ReadLine() does. It pumps a message loop to ensure that messages that Windows sends keep getting dispatched. That message loop does the same thing that Application.Run() does.

Maybe you can see where this is heading, the PropertyGrid is allowed to again call your SendCommand() method. The lock doesn't work at all, this happens on the same thread.

Solving this problem isn't so easy, we can't see the code that gets SendMessage() triggered. But you will need to prevent this from happening, somehow. More background on this behavior in this question.

Answer 2

Where is the field sp assigned? Locks only work on non-null objects.

If sp is assigned differently on each call, then the lock won't be mutually exclusive (locks are only mutually exclusive on the same object instance). In that case, you'd need to have a static field to be used for locking:

private static readonly object _lockObject = new object();

Edit: I see now based on comments in other answers that you are actually running this logic on the UI thread, which is causing the lock to be re-entered multiple times on the same thread (the UI thread) when the message queue is pumped. Run this code on a different thread, and you will gain two advantages: (1) the UI will not lock up as this potentially long-running code executes, and (2) the lock will always be acquired on a new thread, ensuring that subsequent calls to SendCommand will all be on their own thread, and thus enter the lock sequentially as desired.