none
thread safe operation on generic dictionary RRS feed

  • 問題

  • Hi,

    I wrote a server program.

    I use a ClientHandler class to manage incoming clients.

    ClientHandler ch = new ClientHandler();

    there is a thread responsible for sending hearbeat messege to all clients.

    t = new Thread(keepAlive);
    t.Start();

    when server receive a client:

    ch.Add(client_id,new ClientInstance(tcpclient));

    when i move a client from dicClients in ClientHandler

    ch.remove(ci.ClientID);

    invalidoperation exception happens in keepAlive thread.

    foreach (KeyValuePair<int, ClientInstance> kv in dicClients) { kv.Value.sendMessage(MessageCMD.HEART); }

    I should have locked the dicClients. why do I get the Invalidoperation exception when I remove a element from dicClients?

    please help me.

    thank you very much.

    complete code of ClientHandler and ClientInstance are listed below:

    public class ClientHandler { public Dictionary<int, ClientInstance> dicClients = new Dictionary<int, ClientInstance>(); private int client_id; private object lock_obj = new object(); const int HEART_BEAT_LATENCY = 10000; ManualResetEvent modify_client_event = new ManualResetEvent(false); Thread t; public ClientHandler() { t = new Thread(keepAlive); t.Start(); } public static void release() { if (t != null) { t.Abort(); t = null; client_id = 0; } int[] keys = dicClients.Keys.ToArray(); foreach (int key in keys) { remove(key); } } public void add(ClientInstance client) { lock (dicClients) { client_id++; client.ClientID = client_id; dicClients.Add(client_id, client); } } public void remove(int key) { modify_client_event.WaitOne(); lock (dicClients) { if (dicClients.ContainsKey(key)) { dicClients[key].Dispose(); dicClients.Remove(key); } modify_client_event.Reset(); } } private void keepAlive() { while (true) { try { lock (dicClients) { foreach (KeyValuePair<int, ClientInstance> kv in dicClients) { kv.Value.sendMessage(MessageCMD.HEART); } } } catch { Console.WriteLine("invalidoperation"); } modify_client_event.Set(); Thread.Sleep(HEART_BEAT_LATENCY); } } }

    public class ClientInstance {
            TcpClient client;
            Thread t;
            const int THREAD_START_LATENCY = 1000;
            const int WRITE_TIMEOUT = 15000;
            int client_id;
            string user_name;
            DateTime login_time;
            DateTime logout_time;
            ClientHandler ch;
            public int ClientID {
                get {
                    return this.client_id;
                }
                set {
                    this.client_id = value;
                }
            }
            public ClientHandler ClientHandler{
                get {
                    return this.ch;
                }
                set {
                    this.ch = value;
                }
            }
            public event EventHandler<MessageEvent> OnMessage;
            public event EventHandler<StatusEvent> OnStatus;
            public ClientInstance(TcpClient client) {
                this.client = client;
                t = new Thread(readMessage);
                t.Start();
                Thread.Sleep(THREAD_START_LATENCY);
            }
            public void Dispose() {
                if (t != null) {
                    t.Abort();
                    t = null;
                }
                client.Close();
                client = null;
            }
            protected void readMessage() {
                
                NetworkStream ns = null;
                try {
                    byte[] buffer = new byte[client.ReceiveBufferSize];
                    StringBuilder sb = new StringBuilder();
                    int bytes_read = 0;
                    
                    while (true) {
                        Array.Clear(buffer, 0, buffer.Length);
                        bytes_read = 0;
                        sb.Length = 0;
                        ns = client.GetStream();
                        while (ns.DataAvailable) {
                            bytes_read = ns.Read(buffer, 0, client.ReceiveBufferSize);
                            sb.Append(Encoding.ASCII.GetString(buffer, 0, bytes_read));
                        }
                        if (sb.Length != 0) {
                            fireEvent(sb.ToString());
                        }
                    }
                } catch(Exception ex) {
                    Console.WriteLine("ClientInstance:readMessage");
                    Console.WriteLine(ex.StackTrace);
                    if (ns != null) {
                        ns.Close();
                    }
                    ch.remove(this.client_id);//remove client from ClientHandler
                }
            }
            
            public void sendMessage(string msg) {
                NetworkStream ns = null;
                try {
                    ns = client.GetStream();
                    byte[] data = Encoding.ASCII.GetBytes(msg);
                    ns.WriteTimeout = WRITE_TIMEOUT;
                    ns.Write(data, 0, data.Length);
                    fireStatusEvent(StatusType.mtLogMsg, string.Format("{0}:send msg: {1}", ClientID, msg));
                } catch (Exception ex) {
                    Console.WriteLine("send error.ID:{0}", this.client_id);
                    Console.WriteLine(ex.StackTrace);
                    if (ns != null) {
                        ns.Close();
                    }
                    ch.remove(this.client_id);//remove client from ClientHandler
                    } finally {
                    
                }
            }
            protected void fireEvent(string msg) {
                EventHandler<MessageEvent> handler = OnMessage;
                if (handler != null) {
                    handler(this, new MessageEvent(msg));
                }
            }
            protected void fireStatusEvent(StatusType type, string msg) {
                EventHandler<StatusEvent> handler = OnStatus;
                if (handler != null) {
                    handler(this, new StatusEvent(type, msg));
                }
            }
        }

    IDE:VS2008

    .NetFramework3.5


    • 已編輯 mason1939 2012年7月9日 上午 08:32
    • 已移動 Bill ChungMVP, Moderator 2012年7月9日 下午 02:02 一般性 C# 問題 (從:.NET Framework 專業討論區(涵蓋.NET Framework 4 與 .NET Framework 3.x))
    2012年7月9日 上午 08:31

解答

  • 基本上, 這和 lock 應該沒有關係, 主要是因為你在 foreach 中刪除此被列舉集合的元素.

    在 MSDN 文件庫 For Each...Next 陳述式 (Visual Basic) 提到很重要的事 (雖然這是 VB 的,  但這規則適用於 C#)

    修改

    修改集合GetEnumerator 傳回的列舉程式物件通常不允許您經由新增、刪除、取代或重新排列任何項目的方式變更集合。 如果您在初始 For Each...Next 迴圈後變更集合,列舉程式物件會變成無效,而且在下次嘗試存取項目時會引發 InvalidOperationException 例外狀況。

    比較建議的作法是, 你應該另外有一個集合來存放要移除的 key 值. ex: List<T> 以你的例子就是 List<int>, 然後列舉這個 List<T> 去刪除你原來Dictionary 的東西.

    範例如下:

    private void button1_Click(object sender, EventArgs e)
    		{
    			Dictionary<int, string> dic = new Dictionary<int, string>();
    			for (Int32 i = 0; i <= 4; i++)
    			{
    				Byte x = System.Convert.ToByte (65+i);
    				dic.Add(i, System.Text.Encoding.ASCII.GetString(new Byte[] {x}));
    			}
    
    			List<int> removeKey = new List<int>();
    
    			foreach (KeyValuePair<int, string> v in dic)
    			{
    				if (v.Value == "A" || v.Value == "D")
    				{
    					removeKey.Add(v.Key);
    				}
    			}
    
    			foreach (int key in removeKey )
    			{
    				dic.Remove(key);
    			}
    
    			foreach (KeyValuePair<int, string> v in dic)
    			{
    				TextBox1.Text += String.Format("{0} ", v.Value);
    			}
    		}


    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。

    • 已標示為解答 mason1939 2012年7月9日 下午 03:07
    2012年7月9日 上午 10:20
    版主
  • 看起來不是, 不過, 我看不出來需要 WaitHandle 的理由 ?

    如果是我一定是分開做, 如我上面的範例, 先全部送完 heart-beat, 然後發生Timeout Exception 的只記錄下來. 等這個迴圈跑完, 產生另一個迴圈將該 remove 的移除掉.

    另外, 我疏忽了, 你用 Console.Write, 所以應該是主控台程式吧 ? 如果你的 foreach 迴圈很大, 最好是在迴圈內加上 Thread.Sleep(), 最小值可以用 16 會好一點


    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。


    2012年7月9日 下午 05:49
    版主

所有回覆

  • 程式還不短, 錯誤是出現在哪一行 ?

    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。

    2012年7月9日 上午 08:46
    版主
  • Dear Bill:

    錯誤是發生在ClientHandler中的keepAlive method.

    lock (dicClients) {
        foreach (KeyValuePair<int, ClientInstance> kv in dicClients) {
            kv.Value.sendMessage(MessageCMD.HEART);
        }
    }

    在ClientInstance發生寫入網路I/O失敗時,視為client斷線。

    會使用ClientHander中的remove方法將斷線的client移除(每個ClientInstance中會保有一分clienthander,以錯誤發生可以將自己移除)。

    然後就會在foreach(...)這一段發生exception。

    不曉得我使用lock的觀念是否有錯。請您指導,謝謝。

    2012年7月9日 上午 09:10
  • 基本上, 這和 lock 應該沒有關係, 主要是因為你在 foreach 中刪除此被列舉集合的元素.

    在 MSDN 文件庫 For Each...Next 陳述式 (Visual Basic) 提到很重要的事 (雖然這是 VB 的,  但這規則適用於 C#)

    修改

    修改集合GetEnumerator 傳回的列舉程式物件通常不允許您經由新增、刪除、取代或重新排列任何項目的方式變更集合。 如果您在初始 For Each...Next 迴圈後變更集合,列舉程式物件會變成無效,而且在下次嘗試存取項目時會引發 InvalidOperationException 例外狀況。

    比較建議的作法是, 你應該另外有一個集合來存放要移除的 key 值. ex: List<T> 以你的例子就是 List<int>, 然後列舉這個 List<T> 去刪除你原來Dictionary 的東西.

    範例如下:

    private void button1_Click(object sender, EventArgs e)
    		{
    			Dictionary<int, string> dic = new Dictionary<int, string>();
    			for (Int32 i = 0; i <= 4; i++)
    			{
    				Byte x = System.Convert.ToByte (65+i);
    				dic.Add(i, System.Text.Encoding.ASCII.GetString(new Byte[] {x}));
    			}
    
    			List<int> removeKey = new List<int>();
    
    			foreach (KeyValuePair<int, string> v in dic)
    			{
    				if (v.Value == "A" || v.Value == "D")
    				{
    					removeKey.Add(v.Key);
    				}
    			}
    
    			foreach (int key in removeKey )
    			{
    				dic.Remove(key);
    			}
    
    			foreach (KeyValuePair<int, string> v in dic)
    			{
    				TextBox1.Text += String.Format("{0} ", v.Value);
    			}
    		}


    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。

    • 已標示為解答 mason1939 2012年7月9日 下午 03:07
    2012年7月9日 上午 10:20
    版主
  • Dear Bill:

    非常感謝您的解答。我會改一下。

    雖然我沒有在foreach中修改集合,不過因為它是在thread中跑,

    所以可能剛好在foreach中執行時,client斷線後,就會立刻將自己從dicClient中刪除,就造成了在列舉時修改集合。

    在這樣的情況下您所提的解法似乎還是有可能會遇到在列舉時修改集合的狀況

    我判斷client斷線的方法是由keepalive每隔15秒送出一個heartbeat msg

    如果傳回networkstream.write() 發生excption,就表示斷線。

    接著會這這個client刪除。這時有可能是 keepAlive的foreach跑到一半的時候。

    所以我才會想是否可以用lock的方式來改善。也就是我接著所使用同步物件的方式。

    這裡我想請問個延伸的問題:

    之前用lock鎖不住(我想這是我觀念上有問題),

    於是我使用了同步物件來做處理

    我在發文的過程中發現程式的bug,我在使用foreach之前沒有將 ManualResetEvent上鎖。

    改成以下的code之後就不會發生 invalidoperationexception了

            
        modify_client_event.Reset();// Add this
        lock (dicClients) {
                foreach (KeyValuePair<int, ClientInstance> kv in dicClients) {
                kv.Value.sendMessage(MessageCMD.HEART);
        }
    
        modify_client_event.Set();
        Thread.Sleep(HEART_BEAT_LATENCY);


    我的想法是:新增或移除dicClient的元素時,都先等keepAlive這個thread先做跑完。

    不過這造成GUI回應變的很慢,動個游標都卡卡的,

    這樣的方式雖然可work,不過造成GUI效能上的問題。

    想請教一下造成這個問題的發生原因。

    測試時我只用了一個client連進去測試,整個程式最多只有4個thread在跑

    1.accept thread

    2.read network stream thread -- client

    3.read network stream thread -- server

    4.keep alive thread

    而且1,2,3的 thread 理應會block, 第3個thread也是15秒run一次

    應該不致於產生效能的問題。或是是我根本就不該使用這樣的方式呢?

    非常感謝您的回覆。

    ps.如果是使用thread safe的資料結構,lock(xxx.SyncRoot)是否可解決這樣的狀況呢?


    • 已編輯 mason1939 2012年7月9日 下午 03:45
    2012年7月9日 下午 03:07
  • 你該不會是在 UI  Thread 中用了 WaitHandle 吧 ?

    modify_client_event.Reset(); <-- 這個 modify_client_event 的 Waitxxx 是在 UI Thread ?


    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。

    2012年7月9日 下午 03:26
    版主
  • Dear Bill:

    modify_client_event.WaitOne() 是屬於UI Thread該怎麼看呢

    server起始的過程如下:

    UI按一個start的Button,這個button的callback中會new 一個server class的instance

    這時ClientHandler也會一併被new 出來。

    這時ClientHandler會產生一個keepAlive的thread.

    ClientHandler的remove方法的第一行就是modify_client_event.WaitOne();

    不知這樣算不是是產生在UI thread中?(完整程式碼可看之前貼的ClientHandler的class)

    感謝您撥冗回覆。

    2012年7月9日 下午 04:10
  • 看起來不是, 不過, 我看不出來需要 WaitHandle 的理由 ?

    如果是我一定是分開做, 如我上面的範例, 先全部送完 heart-beat, 然後發生Timeout Exception 的只記錄下來. 等這個迴圈跑完, 產生另一個迴圈將該 remove 的移除掉.

    另外, 我疏忽了, 你用 Console.Write, 所以應該是主控台程式吧 ? 如果你的 foreach 迴圈很大, 最好是在迴圈內加上 Thread.Sleep(), 最小值可以用 16 會好一點


    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。


    2012年7月9日 下午 05:49
    版主
  • Dear Bill:

    你是正確的。我是遇到thread就會同步動作開始上腦。

    依您的方法,的確是沒有必要使用同步物件。我想的太複雜了。

    獲益良多,感謝您。

    2012年7月10日 上午 01:02
  • 原則在之前的回應中的MSDN  說明已經很清楚: 不允許新增, 刪除, 取代或重新排列.所以第一個是不要在 foreach 迴圈中自己做自己這件事

    其實這有一個簡單的取巧方法, 就是你的 keepAlive 中用的 集合是"複本". 當你在 Send Heart beat 之前先比對複本是否和正本 (正本就是當你每次 accepted 後就會加東西進來的那個集合), 如果複本和正本不符, 則重新產生複本, 而任何的新增/刪除都對著正本做.


    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。

    2012年7月10日 上午 04:03
    版主
  • Dear Bill:

    好不意思,還有問題想請教。

    那如果是Add到dicClients呢?

    我覺的這也有可能也造keepAlive 中 foreach的invalidopertationexception。

    因為accept thread和keepAlive thread同時會存取dicClients. 

    這邊您建議怎麼處理呢?

    先感謝您的回答。

    2012年7月10日 上午 04:05
  • Dear Bill:

    好不意思,還有問題想請教。

    那如果是Add到dicClients呢?

    我覺的這也有可能也造keepAlive 中 foreach的invalidopertationexception。

    因為accept thread和keepAlive thread同時會存取dicClients. 

    這邊您建議怎麼處理呢?

    先感謝您的回答。


    阿我不是回在上面了.

    在現實生活中,你和誰在一起的確很重要,甚至能改變你的成長軌跡,決定你的人生成敗。 和什麼樣的人在一起,就會有什麼樣的人生。 和勤奮的人在一起,你不會懶惰; 和積極的人在一起,你不會消沈; 與智者同行,你會不同凡響; 與高人為伍,你能登上巔峰。

    2012年7月10日 上午 04:13
    版主
  • sorry,沒更新browser就回覆了,我再試試。感謝
    2012年7月10日 上午 04:52