2

This block of code is being accessed by many threads

    // All code is from same class

    public void ExecuteCommand(IAsciiCommand command, IAsciiCommandSynchronousResponder responder)
    {
        lock (commander)
        {
            if (commander.IsConnected)
            {
                commander.ExecuteCommand(command, responder);
            }
        }
    }

    public void Disconnect()
    {
        var tmp = commander.IsConnected;
        commander.Disconnect();
        if (commander.IsConnected != tmp && !commander.IsConnected)
        {
            OnPropertyChanged("IsConnected");
        }
    }

And eventually i get this: Error

How is this possible, that thread accessed into if statement, whose condition returns false? How can i fix it?

5
  • Where do you close this "port"? Is that code also being locked?
    – vgru
    Aug 12, 2014 at 10:02
  • Service(has connection and port) <--> Model <--> GUI Aug 12, 2014 at 10:03
  • @Groo I'm pretty basic at c# , What's a monitor? Aug 12, 2014 at 10:04
  • But where do you close it? Is there a method which closes this port, without taking a lock? What type of a port is this anyway? (edit) Monitor is the synchronization construct used by the lock statement. My question was basically "do you lock while closing".
    – vgru
    Aug 12, 2014 at 10:04
  • See the updated source Aug 12, 2014 at 10:07

3 Answers 3

2

This is happening because the check and the call lack atomicity. Here is a sequence of events that could lead to an exception:

  • Two threads, A and B, are reaching the condition at the same time
  • Thread A checks the condition, which returns true, so it enters the if block
  • At the same time, thread scheduler decides that thread A has exhausted its time slot, and suspends it
  • Thread B calls Disconnect
  • Thread scheduler resumes thread A, which is inside the if condition. However, the command is no longer connected
  • This causes the exception

You can fix it by locking commander inside Disconnect().

public void Disconnect()
{
    bool doEvent;
    lock(commander) {
        var tmp = commander.IsConnected;
        commander.Disconnect();
        doEvent = (commander.IsConnected != tmp && !commander.IsConnected)
    }
    // Run OnPropertyChanged outside the locked context 
    if (doEvent)
    {
        OnPropertyChanged("IsConnected");
    }
}
1

You need to lock on a static object. Right now you're creating separate locks based on the object your are working with (commander). Try this:

public class WhatEverClassHasTheExecuteCommandMethod
{
     private static object _lock = new object();

     public void ExecuteCommand(IAsciiCommand command, IAsciiCommandSynchronousResponder responder)
     {
          lock (_lock)
              if (commander.IsConnected)                  
                 commander.ExecuteCommand(command, responder);
     }
}
1
  • I don't think OP has multiple commander instances. And if s/he does, I don't see why you would need to lock access to all of them if you are writing to a single one.
    – vgru
    Aug 12, 2014 at 10:12
1

If you are not locking while disconnecting, it's entirely possible to get a race condition. The basic solution is to add a lock inside the Disconnect method:

public void Disconnect()
{
    lock (commander)
    {
        var tmp = commander.IsConnected;
        commander.Disconnect();
        if (commander.IsConnected != tmp && !commander.IsConnected)
            OnPropertyChanged("IsConnected");
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.