C# thread accesed to if block, whose condition returns false

Question

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?

Where do you close this "port"? Is that code also being locked? — vgru, Aug 12 '14 at 10:02
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](http://stackoverflow.com/a/4978886/69809). My question was basically "do you lock while closing". — vgru, Aug 12 '14 at 10:04

Sergey Kalinichenko · Accepted Answer · 2014-08-12T10:14:25.717

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");
    }
}

score 1 · Answer 2 · answered Aug 12 '14 at 10:07

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);
     }
}

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 '14 at 10:12

score 1 · Answer 3 · answered Aug 12 '14 at 10:08

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");
    }
}

C# thread accesed to if block, whose condition returns false

3 Answers3