Thread Safety? Dictionary Locking - I need Help.

[Y u k i]

Well, locking a dictionary seems to be not that clear.

PHP Code:

 Monitor.Enter(AllMobs);
                {
                    foreach (KeyValuePair<uint, Mob> kvp in AllMobs)
                    {
                        Mob m = kvp.Value;
                        m.GetTarget();
                    }
                }
                Monitor.Exit(AllMobs); 


Thats how i do it.

Thats how public sources do it

PHP Code:

 lock(AllMobs)
                {
                    foreach (KeyValuePair<uint, Mob> kvp in AllMobs)
                    {
                        Mob m = kvp.Value;
                        m.GetTarget();
                    }
                } 


Thats how others do it

PHP Code:

private readonly object SyncLock = new object;
 Monitor.Enter(SyncLock);
                {
                    foreach (KeyValuePair<uint, Mob> kvp in AllMobs)
                    {
                        Mob m = kvp.Value;
                        m.GetTarget();
                    }
                }
                Monitor.Exit(SyncLock); 


Version 1: Im getting "Collection Modified" Errors
Version 2: Getting no Errors but the loop lasts sgnificantly longer
Version 3: Getting no Errors but sometimes the loop runs 2 times at once.

Sooo... how do you lock a dict?

[pro4never]

It could be me being drunk but I'd say you MAY be better off using some sort of thread safe collection system.

The general idea is you write a layer to sit ontop of normal dictionaries but when you get the collection you return a COPY of the actual collection.

This means the collection you are iterating through is a copy of the base one and therefor thread issues do not appear.

It's not ideal for ALL solutions but seeing as you keep mentioning monsters... it's one of the more common solutions I've seen.

[Korvacs]

ConcurrentDictionary

Job done.

Also you should use lock() instead of Monitor.Enter/Exit unless you can guarantee that the thread interacting with the dictionary isnt going to crash midway through the lock, lock uses monitor and a try/catch which means that the lock will be released even if a thread crashes while still having ownership of the locked item.

If for some reason you dont want to use ConcurrentDictionary...and i cant think of a legitimate excuse not to in this case, you should use lock() with a sync object, so combine versions 2 and 3, i found however that using lock is an exceptional bottleneck when compared to the ConcurrentDictionary, mainly because lock() blocks where as the ConcurrentDictionary doesn't.

[CptSky]

Originally Posted by Korvacs

ConcurrentDictionary

Job done.

Also you should use lock() instead of Monitor.Enter/Exit unless you can guarantee that the thread interacting with the dictionary isnt going to crash midway through the lock, lock uses monitor and a try/catch which means that the lock will be released even if a thread crashes while still having ownership of the locked item.

If for some reason you dont want to use ConcurrentDictionary...and i cant think of a legitimate excuse not to in this case, you should use lock() with a sync object, so combine versions 2 and 3, i found however that using lock is an exceptional bottleneck when compared to the ConcurrentDictionary, mainly because lock() blocks where as the ConcurrentDictionary doesn't.

I think I'll past to 4.0 just for that class

[{ Angelius }]

how about

PHP Code:

static public Dictionary<T, T2> ThreadSafeDictionary<T, T2>(this Dictionary<T, T2> dictionary)
    {
        Dictionary<T, T2> copy;// = new Dictionary<T, T2>();
        Monitor.Enter(dictionary);
        try
        {
            copy = new Dictionary<T, T2>(dictionary); Monitor.PulseAll(dictionary);
        }
        finally { Monitor.Exit(dictionary); }
        return copy;
    } 


and then do the foreach using the returned copy.

[Korvacs]

See what you've done there is, you've just implemented lock() but manually, and again this method blocks unlike ConcurrentDictionary.

[{ Angelius }]

but how long wold it block ? how long does it take to copy a collection with over than 15k objects ? and how bad it is to use a catch with a Monitor.Exit to ensure that the lock will be released ?

[Korvacs]

It would block for as long as another thread needs it, i dunno try it? I suspect alot longer than it would take to change to the 4.0 framework. Read my first post in this thread for why you should use lock() over Monitor.Enter/Exit, but like i said your doing exactly the same thing as lock only by yourself.

[{ Angelius }]

dont get me wrong cus im not saying that the ConcurrentDictionary's are bad or anything. the point is what i posted up there could be useful not only for Dictionary's but also any other type of collections that needs to be a thread safe collection,

plus Dictionary's are not always the right answer so we can just replace it with a ConcurrentDictionary. there is lists Ilists sortedlists etc most of it is not a thread safe collections so it has to be locked sometimes .

and looking back to that the subject was the Lock/Monitor.Enter that was just my opinion :P

[Korvacs]

Yeah........but the topic is about thread safety when it comes to dictionaries.

[pro4never]

If all you guys want to do is whip it out and see who's bigger, theres more appropriate forums for that.

#cleaned. Stay on topic although I'm sure this could just be closed now, I prefer to not have to close every thread that people decide they want to argue in.

[taylor2846]

ConcurrentDictionary rock =] thanks for that info Korvacs

[BaussHacker]

Not sure, just did this, but don't know if it's correct how I'm thinking 'safe'.

    public class SafeDictionary<TKey, TValue>
    {
        private Dictionary<TKey, TValue> m_dict;
        public SafeDictionary()
        {
            m_dict = new Dictionary<TKey, TValue>();
        }
        public TValue this[TKey Key]
        {
            get
            {
                return m_dict[Key];
            }
            set
            {
                if (ContainsKey(Key))
                    m_dict[Key] = value;
                else
                    Add(Key, value);
            }
        }

        public void Add(TKey Key, TValue Value)
        {
            m_dict.Add(Key, Value);
        }
        public void Remove(TKey Key)
        {
            m_dict.Remove(Key);
        }
        public bool ContainsKey(TKey Key)
        {
            return m_dict.ContainsKey(Key);
        }
        public bool ContainsValue(TValue Value)
        {
            return m_dict.ContainsValue(Value);
        }

        public static implicit operator Dictionary<TKey, TValue>(SafeDictionary<TKey, TValue> dict)
        {
            Dictionary<TKey, TValue> temp = new Dictionary<TKey, TValue>();
            lock (dict.m_dict)
            {
                foreach (TKey key in dict.m_dict.Keys)
                {
                    temp.Add(key, dict.m_dict[key]);
                }
            }
            return temp;
        }

        public static implicit operator TValue[](SafeDictionary<TKey, TValue> dict)
        {
            Dictionary<TKey, TValue> temp = dict;
            return temp.Values.ToArray();
        }

        public int Count
        {
            get
            {
                return m_dict.Count;
            }
        }
    }

[Korvacs]

Originally Posted by Y u k i

I will not use .net 4.0 visual studio 2010 is slow as **** i hate it. I may use Visual studio 11 when its a final but till then ill go with 2008.

Thanks for the idea to return a copy, that will be helpful!

@Korvacs: No i will not use Lock.

lol? Your not going to use .net 4.0 because the compiler is slow? The hell is wrong with you. Concurrent Collections out perform all of your methods your trying at the moment.

And you wont use lock despite the fact that by using monitor you open the source up to threads crashing and not releasing the lock.

Using a copy allows you to work on the dictionary while others work on it yes, but you still have to read from the original and write to it when your creating the copy and when your combining the original with the copy. So you still need to lock in case 2 copys are being combined at the same time, or if your trying to copy back and create a copy at the same time. So a copy doesnt really solve the problem, merely moves it.

Do the right thing and upgrade to 4.0, your sacrificing performance because you wont man up and use that compiler, seriously what the hell lol?

[.Kinshi]

Originally Posted by Y u k i

I will not use .net 4.0 visual studio 2010 is slow as **** i hate it. I may use Visual studio 11 when its a final but till then ill go with 2008.

Thanks for the idea to return a copy, that will be helpful!

@Korvacs: No i will not use Lock.

Get better computer and/or brain.