4
\$\begingroup\$

I have a process running in a separate thread which reads data over Ethernet and raises an event for each read. Then, this data is processed over a tree of classes. At the top of my class tree, I want to store the processed data to SQL tables. This read rate can be rather fast and often at times but on average its slower than my write rate to SQL. This is why I want a queue. Also I want my window not to freeze, so I want a separate thread.

The code below works but I'm fairly inexperienced in multi threading. Is this a good and robust way to accomplish my task?

Private addLogQueue As New Queue
Private dequeueing As Boolean = False
Public Sub addLogHandler(ByVal machineName As String, ByVal LogTime As Date, ByVal EventLog As String, ByVal EventValue As String)
    addLogQueue.Enqueue("INSERT INTO ... some SQL CODE")
    Dim Thread1 As New System.Threading.Thread(AddressOf addLog)
    Thread1.Start
End Sub
Private Sub addLog()
    If Not dequeueing Then
        dequeueing = True
        While addLogQueue.Count <> 0
            Try
                SQLCon.Open()
                SQLCmd = New SqlCommand(addLogQueue.Dequeue(), SQLCon)
                SQLCmd.ExecuteNonQuery()
                SQLCon.Close()
            Catch ex As Exception
                MsgBox(ex.Message)
                If SQLCon.State = ConnectionState.Open Then
                    SQLCon.Close()
                End If
            End Try
        End While
        dequeueing = False
    End If
End Sub

After thinking a bit I realize I might not even need a queue or the dequeueing boolean, can I just start a new thread for every write maybe. I added the queue before I added the multi thread because at times I had to write before the SQLcon is even closed. Since I've added a thread, should I even keep the queue?

\$\endgroup\$
1
  • \$\begingroup\$ You'll likely want to use a lock instead of a boolean, sometimes you can end up with unexpected results. \$\endgroup\$ Commented Jul 9, 2015 at 19:23

1 Answer 1

3
\$\begingroup\$

Single-Threading Style:

Managing resources:

You should be using using to manage your connection. It completely eliminates that... unsightly Try-Catch-Block. Compare following snippet from msdn with a similarly stripped version of your code:

Using connection As New SqlConnection(connectionString)
    connection.Open()
    ' Do work here; connection closed on following line.
End Using

Try
    SQLCon.Open()
    ' Do work here; connection closed on following line.
    SQLCon.Close()
Catch ex As Exception
    MsgBox(ex.Message)
    If SQLCon.State = ConnectionState.Open Then
        SQLCon.Close()
    End If
End Try

Naming

While SQLCon and SQLCmd are ... conventional names, they are a little short and have the air of systems-hungarian... I'd prefer connection and command.

Also Thread1 is really non-speaking, InsertingThread may be the better option.

Whitespace

I like to keep separate Subs clearly outlined by an empty line before starting with [Modifier] Sub ...

Comments and documentation

You may have removed it, but ... this code is extremely undercommented and underdocumented. A comment here and there can help a little wit easing the reading. Then again this code is rather simple and obvious by the names...

Multi-Threading Style:

First off, you currently only allow a single Thread to run... well almost, because of possible interleaving and nonatomic comparison operations.

Consider the following:

Thread 1                   Thread 2
- If Not deququeing        
                           - If Not dequeuing
                               - dequeueing = True
    - dequeueing = True

Suddenly you have 2 threads running and that when you could've had everything easy.

The queue docs mention ConcurrentQueue(Of T) when you "need to access the queue across multiple threads"
This makes the failed attempt at synchronizing with a shared boolean unnecessary and overall improves the code quality.

End result:

After applying my critiques I end up with following code:

Private addLogQueue As New ConcurrentQueue(Of String)

Public Sub addLogHandler(ByVal machineName As String, ByVal LogTime As Date, ByVal EventLog As String, ByVal EventValue As String)
    addLogQueue.Enqueue("INSERT INTO ... some SQL CODE")
    Dim InsertingThread As New System.Threading.Thread(AddressOf addLog)
    InsertingThread.Start
End Sub

Private Sub addLog()
   Using (connection As New SqlConnection(connstring))
        connection.Open()
        Dim statement As String
        While addLogQueue.TryDequeue(out statement)
            command = New SqlCommand(statement, connection)
            command.ExecuteNonQuery()
        End While
    End Using
End Sub
\$\endgroup\$
3
  • \$\begingroup\$ I have one more question though. By using Using I dispose my SQLConnection. If I use the same connection frequently, should I dispose it every time or keep it as a global under my class? Since I do have to declare the same SQLConnection frequently... \$\endgroup\$
    – jazzcool
    Commented Jul 9, 2015 at 20:05
  • 2
    \$\begingroup\$ Using does not dispose the Connection instantly. Especially since you're creating that connection "anew" you probably benefit greatly from the Connection Pooling that's inherently available for SqlConnection. \$\endgroup\$
    – Vogel612
    Commented Jul 9, 2015 at 20:26
  • \$\begingroup\$ Thanks for the explanation. I'm just a Mech guy trying to blend in :) \$\endgroup\$
    – jazzcool
    Commented Jul 9, 2015 at 21:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.