C# - confused on lock

Question

Hello guys, For my network based project I need to lock some codes to prevent simultaneous access. This is my code

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Utility;
using DataBaseConnection;
using System.Net.Sockets;
using System.Data;
using System.IO;

namespace SunHavenClasses
{
    public delegate void CtatReceiveDelegate(string message);
    public class ServerHandlerClass
    {
        public event CtatReceiveDelegate OnChatDataReceive;
        private Settings settings;
        private DBCon con;
        private Utility.Network.Server server;
        private Dictionary<string, Socket> UsersOnline;
        private Dictionary<string, int> unAuthenticatedIps;
        private string pass = "logisoftlogicielbarundipankar";
        public ServerHandlerClass(Settings s)
        {
            settings = s;
            con = s.GetConnection();
            server = new Utility.Network.Server(7777);
            server.ClientConnectEventArise += OnUserConnect;//.OnClientConnect(OnUserConnect);
            server.ClientDataReceiveEventArise += OnUsersDataReceive;
            server.ClientDataSendEventArise += OnDataSendToUser;
            server.ClientDisconnectEventArise += OnUserDisconnect;
            server.OnBlockUser += OnUserBlocked;

            UsersOnline = new Dictionary<string, Socket>();
            unAuthenticatedIps = new Dictionary<string, int>();
        }

        private void OnUserConnect(Utility.Network.ServerEventArguments e)
        {
            Stream data = Utility.Serializing.Serialize(settings);
            data = ZipNEncrypt.Zip(new string[] { "settings" }, new Stream[] { data }, pass);
            server.Send(data, e.ClientSocket);
            //MessageBox.Show(e.ClientSocket.RemoteEndPoint.ToString() + " is Connected!!");
        }
        private void OnUsersDataReceive(Utility.Network.ServerEventArguments e)
        {
            Dictionary<string, System.IO.Stream> data = ZipNEncrypt.Unzip(e.Data, pass);
            User user;
            try
            {
                user = (User)Serializing.Deserialize(data["user"]);
                if (!UsersOnline.ContainsKey(user.GetUserId()))
                {
                    server.BlockIp(e.ClientSocket);
                    return;
                }
                data.Remove("user");
            }
            catch (Exception)
            {
                bool passed = true;
                foreach (string key in data.Keys)
                {
                    if (key.Equals("LoggedIn")) break;
                    string[] str = key.Split('_');
                    if (str[0].Equals("GetData"))
                    {
                        string strr = (string)Serializing.Deserialize(data[key]);
                        if (strr.Contains("Users"))
                        {
                            string ip = e.ClientSocket.RemoteEndPoint.ToString().Split(':')[0];
                            /*CHANGE 1.2.10 00:14*/
                            lock (unAuthenticatedIps)
                            {
                                if (!unAuthenticatedIps.ContainsKey(ip))
                                {
                                    unAuthenticatedIps.Add(ip, 1);
                                }
                                else unAuthenticatedIps[ip] += 1;
                                if (unAuthenticatedIps[ip] >= 11) passed = false;
                            }
                            /*CHANGE 1.2.10 00:14*/
                            break;
                        }
                        else passed = false;//server.AddBlockedIp(ip);
                    }
                    else passed = false;
                }
                if (!passed)
                {
                    server.BlockIp(e.ClientSocket);
                }
            }


            foreach (string key in data.Keys)
            {
                if (key.Equals("LoggedIn"))
                {
                    try
                    {
                        User u = (User)Serializing.Deserialize(data["LoggedIn"]);
                        if (!UsersOnline.ContainsKey(u.GetUserId()))
                        {
                            if (User.ValidateUser(u.GetUserId(), u.GetPassword(), con))
                            {
                                /*CHANGE 1.2.10 00:14*/
                                lock (UsersOnline)
                                {
                                    UsersOnline.Add(u.GetUserId(), e.ClientSocket);
                                    string ip = e.ClientSocket.RemoteEndPoint.ToString().Split(':')[0];
                                    Utility.Log.Write("UserLog.log", u.GetUserId() +
                                        " Logged In From Ip " + ip);
                                }
                                /*CHANGE 1.2.10 00:14*/
                            }
                            else
                            {
                                server.BlockIp(e.ClientSocket);
                                return;
                            }
                        }
                        else
                        {
                            Stream tmpStream = Serializing.Serialize("Same User");
                            tmpStream = ZipNEncrypt.Zip(new string[] { key + "ERROR_SameUser" },
                                        new Stream[] { tmpStream }, pass);
                            server.Send(tmpStream, e.ClientSocket);
                            return;
                        }
                    }
                    catch (Exception) { }
                    return;
                }
                else if (key.Equals("chat"))
                {
                    string ip = e.ClientSocket.RemoteEndPoint.ToString().Split(':')[0];
                    string message = ip + " : "+ (string)Serializing.Deserialize(data[key]);
                    OnChatDataReceive(message);
                    return;
                }
                string[] str = key.Split('_');
                Stream dataStream = null;
                object obj = null;
                try
                {
                    if (str[0].StartsWith("Get"))
                    {
                        if (str[0].Equals("GetData"))
                        {
                            string query = (string)Serializing.Deserialize(data[key]);
                            obj = con.GetData(query);
                        }
                        else if (str[0].Equals("GetColumn"))
                        {
                            string query = (string)Serializing.Deserialize(data[key]);
                            string[] tmp = query.Split('%');
                            obj = con.GetColumn(tmp[0], tmp[1]);
                        }
                        else if (str[0].Equals("GetColumnDistrinctValue"))
                        {
                            string query = (string)Serializing.Deserialize(data[key]);
                            string[] tmp = query.Split('%');
                            obj = con.GetColumnDistrinctValue(tmp[0], tmp[1]);
                        }
                    }
                    else
                    {
                        lock (this)
                        {
                            if (str[0].Equals("ExecuteUpdate"))
                            {
                                if (str[1].Equals("Query"))
                                {
                                    Query query = (Query)Serializing.Deserialize(data[key]);
                                    obj = con.ExecuteUpdate(query);
                                }
                                else if (str[1].Equals("String"))
                                {
                                    string query = (string)Serializing.Deserialize(data[key]);
                                    obj = con.ExecuteUpdate(query);
                                }
                            }
                            else if (str[0].Equals("ExecuteBatchUpdate"))
                            {
                                if (str[1].Equals("Query"))
                                {
                                    Query[] query = (Query[])Serializing.Deserialize(data[key]);
                                    obj = con.ExecuteBatchUpdate(query);
                                }
                                else if (str[1].Equals("String"))
                                {
                                    string[] query = (string[])Serializing.Deserialize(data[key]);
                                    obj = con.ExecuteBatchUpdate(query);
                                }
                            }
                            else if (str[0].Equals("ExecutrInsert"))
                            {
                                Query query = (Query)Serializing.Deserialize(data[key]);
                                obj = con.ExecutrInsert(query);
                            }
                        }
                    }
                    dataStream = Serializing.Serialize(obj);
                    dataStream = ZipNEncrypt.Zip(new string[] { key },
                                new Stream[] { dataStream }, pass);
                }
                catch (Exception ex)
                {
                    dataStream = Serializing.Serialize(ex.Message);
                    dataStream = ZipNEncrypt.Zip(new string[] { key + "_ERROR" },
                                new Stream[] { dataStream }, pass);
                }
                server.Send(dataStream, e.ClientSocket);
            }
        }
        private void OnDataSendToUser(Utility.Network.ServerEventArguments e)
        {
        }
        private void OnUserDisconnect(Utility.Network.ServerEventArguments e)
        {
            //System.Windows.Forms.MessageBox.Show("Disconnected");
            string ip = e.ClientSocket.RemoteEndPoint.ToString().Split(':')[0];
            /*CHANGE 1.2.10 00:14*/
            lock (unAuthenticatedIps)
            {
                if (unAuthenticatedIps.ContainsKey(ip))
                    unAuthenticatedIps.Remove(ip);
            }
            lock (UsersOnline)
            {
                foreach (string key in UsersOnline.Keys)
                    if (UsersOnline[key].Equals(e.ClientSocket))
                    {
                        Utility.Log.Write("UserLog.log", key + " Logged Out From Ip " + ip);
                        UsersOnline.Remove(key);
                        break;
                    }
            }
            /*CHANGE 1.2.10 00:14*/
        }
        private void OnUserBlocked(Utility.Network.ServerEventArguments e)
        {
            string ip = e.ClientSocket.RemoteEndPoint.ToString().Split(':')[0];
            Utility.Log.Write("UserLog.log", "Blocked For Illegal Access From Ip " + ip);
        }
        public void Send(Stream dataStream)
        {
            foreach (string key in UsersOnline.Keys)
            {
                try
                {
                    server.Send(dataStream, UsersOnline[key]);
                }
                catch (Exception) { }
            }
        }
        public void Send(Stream dataStream, Socket client)
        {
            try
            {
                server.Send(dataStream, client);
            }
            catch (Exception) { }
        }
        /*changed*/
        public bool AddUser(string userId, Socket socket)
        {
            if (UsersOnline.ContainsKey(userId)) return false;
            UsersOnline.Add(userId, null);
            return true;
        }
        public void RemoveUser(string userId)
        {
            if (!UsersOnline.ContainsKey(userId) || UsersOnline[userId] != null) return;
            UsersOnline.Remove(userId);
        }
    }
}

Now I am not sure that I am using lock correctly.Please Give me some advice. Thanks.

Answer 1

You are using it correctly some of the time. unAuthenticatedIps is being protected correctly, but UsersOnline is not. Let's consider two parallel threads passing through your code:

                                             Thread A           Thread B
                                             --------           --------
if (!UsersOnline.ContainsKey(u.GetUserId())  Statement's true   Statement's true
{
    lock (UsersOnline)                       Get lock           Block
    {
        UsersOnline.Add                      UsersOnline.Add
    }                                        Release Lock
}                                                               Get lock
                                                                UsersOnline.Add
                                                                Release lock   

Notice that both threads A and B modify the UsersOnline dictionary. This structure would properly protect that object:

if (!UsersOnline.ContainsKey(u.GetUserId()))
{
    if (User.ValidateUser(u.GetUserId(), u.GetPassword(), con))
    {
        lock (UsersOnline)
        {
            // Note the additional check in case another thread
            // added this already
            if (!UsersOnline.ContainsKey(u.GetUserId())
            {
                UsersOnline.Add(u.GetUserId(), e.ClientSocket);
                // ...
            }
        }
    }
    else
    {
        server.BlockIp(e.ClientSocket);
        return;
    }
}

As far as the last lock goes (lock (this)), I don't yet see why you would need it. str and obj are both local variables, so you shouldn't need to worry about them being modified by separate threads. And, as other have stated, locking this is not recommended.

Answer 2

I'm guessing you read a lot more than you write? If so, a ReaderWriterLockSlim may be more appropriate to reduce blocking (take read when you just want to check for the key, and write to manipulate the data).

By that I mean you could do double-checked locking with a read at first, then if it fails take a write lock, check again and add if necessary.

Also - the lock(this) is generally frowned upon; having a separate lock object is preferred.

Note that to be effective, all access must respect the lock; there are some places where UsersOnline is locked, and some places where it is accessed without a lock, for example; those second cases may explode in a gooey mess.

For example:

if (!UsersOnline.ContainsKey(u.GetUserId()))
{
    if (User.ValidateUser(u.GetUserId(), u.GetPassword(), con))
    {
        /*CHANGE 1.2.10 00:14*/
        lock (UsersOnline)
        {
            UsersOnline.Add(u.GetUserId(), e.ClientSocket);

In the above, if it is possible that two threads are looking at UsersOnline, then you've already failed by attempting ContainsKey without the lock. If another thread is mutating the state when you do this.... boom.

Answer 3

First of all, you code isn't thread safe at all. In your code you locks only modifying operations (Remove, Add), but also you should lock all access to shared fields. Actually this code not thread safe at all. I think that in this case ReaderWriterLockSlim - would be the best choise.

Second. lock(this) - very bed idea. You should use special objects for this.

Finally, I think your code is messy and hard to understand. Maybe your class solve many different tasks. Maybe you should extract some logic to separate classes (for example create guarded dictionaries as separate classes) or something else.

Sample of using ReaderWriterLockSlim:

someSharedResource;
someSharedResourceRWLock = new ReaderWriterLockSlim();

Some reading code:

try
{
   someSharedResourceRWLock.EnterReadLock();
   //access to someSharedResource for reading
}
finally
{
   someSharedResourceRWLock.ExitReadLock();
}

Some writing code:

try
{
   someSharedResourceRWLock.EnterWriteLock();
   //access to someSharedResource for modifications
}
finally
{
   someSharedResourceRWLock.ExitWriteLock();
}

Answer 4

The advice given so far has been great, but I have a design suggestion you may want to consider. Replace this:

private Dictionary<string, Socket> UsersOnline; 

with a custom Thread Safe Dictionary

private ThreadSafeDictionary<string, Socket> UsersOnline

If its a fit for your needs, it would be a nice way to separate the business logic from the threading logic.