Threading test question

Question

I recently had a interview question in a test that was similar to the below, I do not have very much experience of development using threads can someone please help advise me how to approach this question?:

public class StringQueue
{
    private object _lockObject = new object();

    private List<string> _items = new List<string>();

    public bool IsEmpty()
    {
        lock (_lockObject)
            return _items.Count == 0;
    }

    public void Enqueue(string item)
    {
        lock (_lockObject)
            _items.Add(item);
    }

    public string Dequeue()
    {
        lock (_lockObject)
        {
            string result = _items[0];
            _items.RemoveAt(0);

            return result;
        }
    }
}

Is the following method thread safe with the above implementation and why?

public string DequeueOrNull()
{
    if (IsEmpty())
        return null;

    return Dequeue();
}

Answer 1

It seems to me the answer is no.

While isEmpty() procedure locks the object, it is released as soon as the call is returned - a different thread could potentially call DequeueOrNull() between the call to IsEmpty() and Dequeue() (at which time the object is unlocked), thus removing the only item that existed, making Dequeue() invalid at that time.

A plausible fix would be to put the lock over both statements in DequeueOrNull(), so no other thread could call DeQueue() after the check but before the DeQueue().

Answer 2

It is not threadsafe. At the marked line it is possible that the Dequeue method is called from another thread and thus, the consequent Dequeue return a wrong value:

public string DequeueOrNull()
{
    if (IsEmpty())
        return null;
///  << it is possible that the Dequeue is called from another thread here.
    return Dequeue();
}

The thread safe code would be:

public string DequeueOrNull()
{
  lock(_lockObject) {
    if (IsEmpty())
        return null;
    return Dequeue();
  }
}

Answer 3

No, because the state of _items could potentially change between the thread-safe IsEmpty() and the thread-safe Dequeue() calls.

Fix it with something like the following, which ensures that _items is locked during the whole operation:

public string DequeueOrNull()
{
  lock (_lockObject)
  {
    if (IsEmpty())
      return null;

    return Dequeue();
  }
}

Note: depending in the implementation of _lock, you may wish to avoid double-locking the resource by moving the guts of IsEmpty() and Dequeue into separate helper functions.