Lock on Static List or access by Key

Question

Please give expert opinion, refer to below static sorted list based on key value pair.

Method1 for close connection uses approach of accessing sorted list using key.

Method2 for close connection uses lock statement on the Sorted List and access it by index.

Please guide which approach is better as thousands of users simultaneously creating thousands of connections on web application. Note, accessing by index without locking can raise Index out of bound exception.

internal class ConnA
{
    static internal SortedList slCons = new SortedList();   

    internal static bool CreateCon(string ConnID)
    {
        string constring = "sqlconnectionstring_containing_DataSource_UserInfo_InitialCatalog";         
        SqlConnection objSqlCon = new SqlConnection(constring);
        objSqlCon.Open();
        bool connSuccess = (objSqlCon.State == ConnectionState.Open) ? true : false;

        if (connSuccess && slCons.ContainsKey(ConnID) == false)
        {
            slCons.Add(ConnID, objSqlCon);
        }
        return connSuccess;
    }

    //Method1
    internal static void CloseConnection(string ConnID)
    {
        if (slCons.ContainsKey(ConnID))
        {
            SqlConnection objSqlCon = slCons[ConnID] as SqlConnection; 
            objSqlCon.Close();
            objSqlCon.Dispose();
            objSqlCon.ResetStatistics();
            slCons.Remove(ConnID);
        }
    }

    //Method2
    internal static void CloseConnection(string ConnID)
    {
        lock (slCons)
        {
            int nIndex = slCons.IndexOfKey(ConnID);
            if (nIndex != -1)
            {
                SqlConnection objSqlCon = (SqlConnection)slCons.GetByIndex(nIndex);
                objSqlCon.Close();
                objSqlCon.Dispose();
                objSqlCon.ResetStatistics();
                slCons.RemoveAt(nIndex);
            }
        }       
    }

internal class UserA
{
    public string ConnectionID { get { return HttpContext.Current.Session.SessionID; } }

    private ConnA objConnA = new objConnA();

    public void ConnectDB()
    {
        objConnA.CreateCon(ConnectionID));            
    }

    public void DisConnectDB()
    {
        objConnA.CloseConnection(ConnectionID));            
    }
}

Do not store database connections in static lists. You are manually doing some sort of connection pooling, dont reinvent the wheel and let the .net framework release its resources when they are not used anymore. — Cleptus, Sep 17 '19 at 10:47
Do not lock on slCons: prefer to create a unique object named for example "object locker = new object();" and lock it and do nothing else with it. — , Sep 17 '19 at 10:49
Related question [Is using a singleton for the connection a good idea in ASP.NET website?](https://stackoverflow.com/questions/1557592/is-using-a-singleton-for-the-connection-a-good-idea-in-asp-net-website) — Cleptus, Sep 17 '19 at 10:50
Whenever you have a list that is shared by many processes you always have to lock when adding/deleting to prevent sharing issues like getting out of index error. The process can switch in the middle of the add/delete and you end up accessing the wrong index. — jdweng, Sep 17 '19 at 10:50
@jdweng, Right. Do we need to lock even when we are accessing through key? — Hassan, Sep 17 '19 at 10:55
Yes because another process could be removing the key while you are attempting to read the key. So you may find the key but then when reading the value it is removed and becomes null. — jdweng, Sep 17 '19 at 11:02
@jdweng, one user in the scenario cannot have multiple keys. For a user Key will be linked with unique session identity. — Hassan, Sep 17 '19 at 11:07
@Hassan : WHAT?? If you have a shared dictionary between processes every process is sharing the keys. — jdweng, Sep 17 '19 at 11:12
@jdweng. Key is User Session ID, which is unique for every user. Value is the sql connection. — Hassan, Sep 17 '19 at 11:39
@Hassan: Who is talking about users. A multi-threaded application is the same user. — jdweng, Sep 17 '19 at 11:43
This is an XY problem. The question - X - is how to use a static class to ensure that for every session ID there is a single SQL connection. The Y is why must every session ID have a single SQL connection? I'll never say there's never a good reason, but that sounds wrong. Why do you want to do that? I can 99% guarantee that there's a simpler, better way to do what you're trying to do, or that you don't need to do it at all. — Scott Hannen, Sep 17 '19 at 14:49

score 1 · Answer 1 · answered Sep 17 '19 at 13:50

Access to the SortedList isn't thread safe.

In CreateCon, two threads could access this simultaneously:

if (connSuccess && slCons.ContainsKey(ConnID) == false)

Both threads could determine that the key isn't present, and then both threads try to add it, so that one of them fails.

In method 2:

When this is called - slCons.RemoveAt(nIndex); - the lock guarantees that another call to the same method won't remove another connection, which is good. But nothing guarantees that another thread won't call CreateCon and insert a new connection string, changing the indexes so that nIndex now refers to a different item in the collection. You would end up closing, disposing, and deleting the wrong connection string, likely one that another thread was still using.

It looks like you're attempting an orchestration which ensures that a single connection string will be used across multiple operations. But there's no need to introduce that complication. Whatever class or method needs a connection, there's no need for it to collaborate with this collection and these methods. You can just let each of them open a connection when it needs it and dispose the connection when it's done.

That's expensive, but that's why the framework implements connection pooling. From the perspective of your code connections are being created, opened, closed, and disposed.

But behind the scenes, the "closed" connection isn't really closed, at least not right away. It's actually kept open. If, in a brief period, you "open" another connection with the same connection string, you're actually getting the same connection again, which is still open. That's how the number of connections opened and closed is reduced without us having to manually manage it.

That in turn prevents us from having to do what it looks like you're doing. This might be different if we were opening a transaction on a connection, and then we had to ensure that multiple operations were performed on the same connection. But even then it would likely be clearer and simpler to pass around the connection, not an ID.

Thank you for detailed answer. What if i have `lock(slCons)` in `CreateCon` method before adding new item in the list. — Hassan, Sep 25 '19 at 06:16

Lock on Static List or access by Key

1 Answers1