Issues in the ConcurrentHeap implementation

Question

Here is my MaxHeap implementation. Most of the time it works but occasionally it just causes few values out of order. I want it to be threadsafe. Tried debugging many times but couldn't identify the issue. Can anyone please point out the issues in my implementation?

using System.Threading;
using Crossbones.Common;
using LanguageExt;
using static LanguageExt.Prelude;

namespace MySpace
{

public static class Operators
{
    public static bool XOR(bool x, bool y) => (x && !y) || (!x && y);
    public static bool XAND(bool x, bool y) => (x || !y) && (!x || y);
}
    public enum CompareResult
    {
        Left = 1,
        Equal = 0,
        Right = -1
    };

    public delegate CompareResult HeapComparer<T>(in T left, in T right);

    public class Heap<T> where T : struct
    {
        private readonly int _capacity;
        private readonly IComparer<T> _comparer;
        private readonly List<T> _list;
        private long _itemCount;

        public Heap(int capacity, IComparer<T> comparer)
        {
            _capacity = capacity;
            _comparer = comparer;
            _list = new List<T>(capacity);
        }

        public Option<T> Pop()
        {
            Option<T> head = None;


            lock (_list)
            {
                if (_list.Any())
                {
                    head = Some(_list[0]);
                    _list[0] = _list[^1];
                    _list.RemoveAt(_list.Count - 1);
                    Count = _list.Count;
                    ShiftDown(0);
                }
            }


            return head;
        }
        public void Push(in T item)
        {
            lock (_list)
            {
                _list.Add(item);
                var itemIdx = _list.Count - 1;
                while (itemIdx > 0)
                {
                    var parentIdx = (int)(itemIdx - 1) / 2;
                    if (Compare(parentIdx, itemIdx) == CompareResult.Right)
                    {
                        Swap(parentIdx, itemIdx);
                        itemIdx = parentIdx;
                    }
                    else break;
                }

                Count = _list.Count;
            }
        }

        public bool IsEmpty => Count ==0;
        public bool IsFull => Count == _capacity;
        public long Count
        {
            get => Interlocked.Read(ref _itemCount);
            private set => Interlocked.Exchange(ref _itemCount, value);
        }

        public Option<T> Peek(int position)
        {
            Option<T> ret = None;

            if (position < Count - 1)
            {
                lock (_list)
                {
                    ret = Some(_list[position]);
                }
            }

            return ret;

        }

        public IEnumerable<T> ReadAll()
        {
            var ret = new List<T>((int)Count);

            lock (_list)
            {
                ret = _list;
                _list.Clear();
                Count = 0L;
            }

            return ret;
        }

        void Swap(int leftIdx, int rightIdx)
        {
            var tmp = _list[leftIdx];
            _list[leftIdx] = _list[rightIdx];
            _list[rightIdx] = tmp;
        }
        CompareResult Compare(int idxLeft, int idxRight) => _comparer.Compare(_list[idxLeft], _list[idxRight]) switch
        {
            -1 => CompareResult.Right,
            0 => CompareResult.Equal,
            1 => CompareResult.Left
        };
        int ShiftDown(int itemIdx)
        {
            var ret = itemIdx;
            var maxIdx = _list.Count - 1;
            while (itemIdx < maxIdx)
            {
                var left = (index: 2 * itemIdx + 1, present: (2 * itemIdx + 1) <= maxIdx);
                var right = (index: 2 * itemIdx + 2, present: (2 * itemIdx + 2) <= maxIdx);

                if (left.present && right.present)
                {
                    var target = Compare(left.index, right.index) switch
                    {
                        CompareResult.Left => left,
                        CompareResult.Right => right,
                        CompareResult.Equal => left
                    };
                    if (target.present)
                    {
                        Swap(target.index, itemIdx);
                        itemIdx = target.index;
                    }
                    else break;
                }
                else if (Operators.XOR(left.present, right.present))
                {
                    var target = left.present ? left : right.present ? right : (index: itemIdx, present: false);
                    if (target.present && Compare(target.index, itemIdx) == CompareResult.Left)
                    {
                        Swap(target.index, itemIdx);
                        itemIdx = target.index;
                    }
                    else itemIdx = target.index;
                }

                else break;
            }

            return ret;
        }
    }

}

I use LanguageExt.Core for some functional stuff. mainly Option, Some, None.

I'm keeping the count separately so that reading the Count and IsEmpty, IsFull are atomic operations.

Why not use [`ConcurrentStack`](https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentstack-1?view=netframework-4.8)? — , Dec 17 '19 at 11:52
You haven't specified the scenarios of inconsistency but as hot spots: `Peek` method isn't atomic and its implementation may tolerate the situations when `Count` and the '_list' size are desynchronized. Also `ReadAll` returns the reference to internal collection which can be mutated by other methods while consumers can be not aware of that. — Dmytro Mukalov, Dec 17 '19 at 12:32
ConcurrentStack does not let manipulate internal structure, I need that because I want a priority queue. Hence I use a list with locks — Simple Fellow, Dec 30 '19 at 08:50
@Dmytro Mukalov you are correct, I changed the implementation of ReadAll() — Simple Fellow, Dec 30 '19 at 08:51

score 3 · Answer 1 · answered Dec 17 '19 at 13:59

3

I can point out a few issues:

Count, get => Interlocked.Read(ref _itemCount); You don't use a lock. As a result, this can easily return an incorrect value, if a thread calls it while another is in Push, just between adding the new element and updating the count. Same goes for Pop. Hence, IsFull and IsEmpty will also fail.
Peek - you read Count out of the lock, so it first decide there is an item in the heap and try to get it, but there isn't any already.
ReadAll looks broken. Unless I am mistaken, it will always return an empty list. You properly try to return a copy, but I don't see you making a copy. You copy the reference.

Depending on your use cases, you might want to look and ReaderWriterLockSlim instead of lock.

answered Dec 17 '19 at 13:59

Nick

4,787
2
18
24

aren't Interlocked set of functions atomic? so atomic instructions dont need locks? I'm using lock bcoz it's faster. – Simple Fellow Dec 17 '19 at 15:05
since we are moving the reference in readAll(), and the old reference is sent out while the list is initialized as New reference so I think it should not matter bcoz all the locking and reading writing will be against the new reference and not the old one. What do you think? Actually the actual heap also implementation seems broken or I have missed something – Simple Fellow Dec 17 '19 at 15:07
@SimpleFellow, it doesn't matter what the lock is issued against - you essentially can use totally different object for that, what really matters is what you're trying to protect by the lock and you are merely exposing the instance which can be subject of concurrent access without any protection of that. – Dmytro Mukalov Dec 17 '19 at 15:12
@SimpleFellow, you code: 'ret = _list; _list.Clear();` means you are clearing the same instance you return and you will return an empty list. – Nick Dec 17 '19 at 16:36
@SimpleFellow, Intelocked functions are atomic, but you have a race condition between some code running `Count => get` and some code running `Push` or `Pop`. When you call `Count => get`, you can get a value that doesn't reflect the actual number of items in your list. – Nick Dec 17 '19 at 16:38

score 1 · Accepted Answer · answered Dec 18 '19 at 05:12

The logic for ShiftDown is overly complex. When left and right are both present, there's no need to test target.present. You already know that target is present, since it can only be left or right.

And in the case when they're not both present, then you know that left is the one that's present. In a proper heap, because it's left-filled, a node can't have a right child if it doesn't have a left child.

A much simpler implementation is below. I maintained the return value, although I don't see why you would need this method to return a value. You certainly don't use it in your code.

int ShiftDown(int itemIdx)
{
    var ret = itemIdx;
    var  maxIdx = _list.Count - 1;
    while (itemIdx < maxIdx)
    {
        var largestChild = itemIdx;
        var leftIdx = 2*itemIdx + 1;

        if (leftIdx > maxIdx)
        {
            // node has no children
            break;
        }

        // left child exists. See if it's larger than its parent.
        if (_list[itemIdx > _list[itemIdx]) 
        {
            largestChild = leftIdx;
        }
        var rightIdx = leftIdx + 1;
        if (rightIdx <= maxIdx && _list[rightIdx] > _list[largestChild])
        {
            // right child exists and is larger than current largest child.
            largestChild = rightIdx;
        }
        if (largestChild != itemIdx)
        {
            Swap(itemIdx, largestChild);
            itemIdx = largestChild;
        }
    }
    return ret;
}

actually your implementation works faster. I had to make a few changes though — Simple Fellow, Dec 23 '19 at 07:15

Issues in the ConcurrentHeap implementation

2 Answers2