.NET multithreading Synclock vs Monitor

Question

I'm building a slightly more efficient connection pool for a multi threaded service I'm writing to talk between my service and its SQL database. basically boiled down, I've got this code in my class:

Public Class DBMC
Private Connections As New ArrayList

  Private Function GetConnection() As Object
    Dim conCounter As Integer = 0

    While True
        If Connections.Count > conCounter Then
            If System.Threading.Monitor.TryEnter(Connections(conCounter), 10) Then
                Return Connections(conCounter)
            Else
                conCounter += 1
            End If

        ElseIf conCounter > 98 Then
            'keep looping until an open connection is found
            conCounter = 0

        ElseIf conCounter < 98 Then
            Dim connection As Object = NewCon()

            If connection Is Nothing Then
                conCounter = 0
            Else
                Connections.Add(connection)

                Return connection
            End If
        End If
    End While
    'below line should never run
    Return Nothing
  End Function

  Public Function DBSelect(ByVal SQL As String) As DataSet
    Dim connection As Object = GetConnection()

    SyncLock (connection)
        'Run the select vs database and do a bunch of other stuff
    End SyncLock

    Try
        System.Threading.Monitor.Exit(connection)
    Catch ex As Exception
    End Try

    Return DataSet
  End Function
End Class

So this code works absolutely great, I can run 500 select statements in different threads as fast as the computer can make them and it will open 8 different connections (likely due to the speed of my computer, a slower PC may open more). The thing is, in the DBSelect function, I have a line surrounded in Try/Catch to release the monitor Enter because sometimes ending the synclock drops the locks on my objects (in this case the line is not needed and throws an exception) and sometimes the object is still locked and would stay permanently locked without running that line (in which case it uses it and passes successfully). I cannot figure out for the life of me why Sometimes it releases it, and sometimes it doesn't. Any ideas?

score 2 · Accepted Answer · answered Jun 29 '11 at 23:57

2

The reason you're getting an exception is that, when you have no available connections, you create a new connection and return it without calling Monitor.Enter() on it. This means that you'll take a non-recursive SyncLock to that object reference, release that lock and then try to call Monitor.Exit() on an additional lock that you never took in the first place.

You also have a potential race condition surrounding the way you're adding connections to the pool. Another thread could very well take a lock to the connection that you just created (through the Monitor.TryEnter() call) before you ever make it the SyncLock block to take it yourself. If you're closing the connection before returning it to the pool (which is a good idea) then when your creating thread actually gets to use it then you'll have a connection in a bad state.

I would actually suggest that you don't try to write your own connection pool. Your current code has nothing in it that suggests that you couldn't just use System.Data.SqlClient.SqlConnection instead, which already handles connection pooling for you.

answered Jun 29 '11 at 23:57

Chris Hannon

4,134
1
21
26

that class does do connection pooling, however it only opens 1 connection per unique connection string at least so says the Microsoft documentation for it. So the reason it sometimes throws the exception is when it makes a new one and I don't use the monitor to lock it, i knew there was something important i was missing. At what point is it more beneficial to close the connection before returning it to the pool? – Jrud Jun 30 '11 at 13:53
1

I think you misread, it creates a single _pool_ per unique connection string. Each pool has a default of 100 potential connections. [Here](http://msdn.microsoft.com/en-us/library/8xx3tyca.aspx) is a link to the MSDN documentation, check out the section that's titled Pool Creation And Assignment. – Chris Hannon Jun 30 '11 at 15:45
1

As for closing the connection, you want to initialize resources you took out and clean them up when you put them back. If your connection got into a bad state, for whatever reason, and you put it back into the pool for reuse, that guarantees that other code needing a good connection will have a problem that they weren't responsible for and had no way of knowing about. Additionally, that means that, regardless of the actual amount of use, you have N connections to SQL Server sitting around that are just taking up space and monopolizing the available SQL Server connections. – Chris Hannon Jun 30 '11 at 15:49
I don't care what that MSDN documentation says, if two threads try to access my sql connection object at the same time, 1 of them throws an exception letting me know that another thread is actively using it leading me to believe that the object is not thread safe. – Jrud Jul 22 '11 at 14:19
1

@Jrud That's absolutely correct, the SqlConnection instance is _not threadsafe_ and is stated as such in the documentation for SqlConnection. That's why creating a new SqlConnection instance _for each time you need a connection_ is how it should be used. Each instance will use the same underlying pool to pull the connection from, but you should never share the instances between threads. – Chris Hannon Jul 22 '11 at 22:50
So every time I need to communicate with a database I need to create a SQL connection object, a data adapter, potentially a dataset, and then run a database command? That seems so inefficient to me. Why not just have a few objects and pass them around rather than constantly reinstatiating new ones to do the exact same calls? – Jrud Jul 23 '11 at 00:11
1

@Jrud That really depends on your definition of 'inefficient', which is vague and unspecified. Non-performant for your specific needs? Memory requirements outweigh synchronization costs? If you want to get into that discussion, I suggest asking that as a new question on Stack Overflow with a clear definition. – Chris Hannon Jul 25 '11 at 18:50

.NET multithreading Synclock vs Monitor

1 Answers1