.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?

Answer 1

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.