StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

POConcurrency problems. Locks not released
text
Body
copied!<p>Hi there I got some headaches while working on my project.<br> Short summary:<br></p> <ul> <li>Client/server application (I'm on the server side)</li> <li>Multithreaded</li> <li>KeepAlive every second with System.Timers.Timer</li> <li>Main networking loop on separate thread (reads/writes packets to/from clients)</li> <li>Server on separate thread (doesn't matter at this point)</li> </ul> <p>I have a ClientHandler class which handles all clients. (Networker is the main loop) <img src="https://i.stack.imgur.com/Aacr3.png" alt="CodeMap of ClientHandler"> ClientList is implemented as this:</p> <pre><code>public List<Client> ClientList { get; private set; } </code></pre> <p>Everytime I try to access ClientList (read/write) I use...</p> <pre><code>lock(ClientList){} lock(ClientHandler.ClientList){} </code></pre> <p>... depending where if I'm in or outside ClientHandler.</p> <p>Up to now I didn't use any locks so there were some concurrency problems.<br> But now as I'm using / misusing locks I got some problems with the keepalives.<br> If a client connects:</p> <pre><code>public bool AddClient(Client client) { lock (ClientList) { if (client == null || ClientList.Contains(client)) return false; ClientList.Add(client); return true; } } </code></pre> <p>And every second my timer enqueues a keepalive:</p> <pre><code>private void KeepAliveTimer_Elapsed(object sender, ElapsedEventArgs e) { KeepAliveTimer.Stop(); lock (ClientList) { if (ClientList.Count > 0) { foreach (Client client in ClientList) { lock (client) { client.PacketQueue.Enqueue(new KeepAlivePacket()); } } } } KeepAliveTimer.Start(); } </code></pre> <p>And my current main loop:</p> <pre><code>private void Networker() { while (IsRunning) { lock (ClientHandler.ClientList) { if (ClientHandler.ClientList.Count > 0) { foreach (Client client in ClientHandler.ClientList) { // Check if client has data to read. // Read for up to 10 msecs. if (client.DataAvailable) { DateTime expiry = DateTime.Now.AddMilliseconds(10); while (DateTime.Now <= expiry) { int id = client.ReadByte(); if (id == -1 || !PacketHandler.HandlePacket((byte)id, client, this)) { ClientHandler.DisconnectClient(client); continue; } } } // Check if client has data to write. // Write for up to 10 msecs. if (client.PacketQueue.Count > 0) { DateTime expiry = DateTime.Now.AddMilliseconds(10); while (DateTime.Now <= expiry && client.PacketQueue.Count > 0) { IPacket packet = client.PacketQueue.Dequeue(); if (!packet.Write(client)) { ClientHandler.DisconnectClient(client); continue; } } } } } } Thread.Sleep(1); } } </code></pre> <p>Before all these locks my test client got a KeepAlivePacket every second.<br> Now I'm getting it only once because after the first KeepAlivePacket KeepAliveTimer_Elapsed can't access the lock anymore because its permanently locked by some other thread (tested it with some debug output).</p> <p>Is there something in the provided code that could be the mad guy or is there something else I'm doing completely wrong?<br></p> <p>Would be great if someone could get me out of this misery.</p> <p>Edit (thanks to Joachim Isaksson):<br> I don't know if it was the only bug but one thing I forgot is to check in the mainloop if there is data available after I read the first packet.<br> That was the first problem because I only send one packet with my TestClient and the server got stuck at client.ReadByte because there was no check beforehand.</p> <pre><code>if (client.DataAvailable) { DateTime expiry = DateTime.Now.AddMilliseconds(10); while (DateTime.Now <= expiry && client.DataAvailable) { try { int id = client.ReadByte(); // do stuff... }... } } </code></pre>

Querying!

Guidance

An individual column

Larger individual text columns get their own page to allow for proper reading.

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload