5
\$\begingroup\$

I have developed a test app that works by getting webpages source and processing it. I have up to 100k URLs in queue and want to use a maximum of 25 threads. Please help me check if the code is good or point out any faults you might find.

Public Class frmMain
    Public Delegate Sub AddItemDelegate(ByVal Item As String, ByVal Status As String)
    Private TotalItems As Integer =0

    Public Sub AddItem(ByVal item As String, ByVal status As String)

        If Me.InvokeRequired Then
            Me.Invoke(New AddItemDelegate(AddressOf AddItem), Item, Status)
        Else
            Dim objLock As Object = New Object()
            Dim lvItem As New ListViewItem

            With lvItem
                .Text = Item
                If Status.Length < 200 Then
                    .SubItems.Add(Status)
                Else
                    .SubItems.Add(Status.Length.ToString)
                End If
            End With

            If listView1.Items.Count <TotalItems-1 Then
                listView1.Items.Add(lvItem)
            Else
                tsslstatus.Text ="Done"
                btnStart.Enabled =true
            End If
        End If
    End Sub

    Public Sub DoWork(ByVal objItem As Object)
        static _sem as Semaphore = new Semaphore(5,10)  
        Dim objUrl as String = DirectCast(objItem, string)

        Try
            If objUrl Is Nothing Then
                exit sub
            End If

            Dim URL as New Uri(objUrl)
            _sem.waitone()
            AddItem(URL.AbsoluteUri,gethtml(URL))
            _sem.Release()
        Catch ex As Exception
            debug.Print("Error " & ex.StackTrace )
        End Try
    End Sub

    Private Sub btnStart_Click(ByVal sender As System.Object, _
        ByVal e As System.EventArgs) Handles btnStart.Click
        btnStart.Enabled =False
        dim blnResult as Boolean = ThreadPool.SetMaxThreads(25, 25)
        System.Threading.Thread.Sleep(1000)
        listView1.Items.Clear

        For Each sItem As String In txturls.Lines
            if sItem.Trim <>string.empty
                ' Queue a task
                If sItem.Contains("http://")=False Then
                    sItem="http://" & sItem
                End If

                ThreadPool.QueueUserWorkItem(New WaitCallback(AddressOf DoWork), sItem)
                TotalItems+=1
                tsslTotal.Text =String.Format ("Total Tasks: {0}",TotalItems)
            end if
        Next
    End Sub
End Class
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • Relying on the max number of thread pool threads to cap the number of concurrent downloads feels ... wrong. It may also cause problems if another component needs a thread pool while all 25 are waiting for a response from web servers.

    (slight correction) Although you use semaphores to limit the number of concurrent downloads to ten, you will still have up to 25 thread pool threads in the DoWork() method (15 blocked on the semaphore).

    Two alternatives:

    • Manage you own task list and threads (since 4k URLs per thread could take more than a few seconds).
    • Make your requests asynchronously and track the list of IAsyncResult objects (though this may get a little fiddly).
  • I'm not sure I like the idea of using a static Semaphore to protect instance data, you should store the semaphore in a local field. Even if you only ever have a single instance, I prefer to scope the lock similarly to the data to be protected. (sorry, I thought the semaphore was to protect the controls rather than used as a throttle)

  • You should release your semaphore in a finally block.

  • Only the thread that created a control should be used to update it. I recommend using ISynchronizeInvoke.Invoke() to call AddItem() (implemented implicitly on Control). This will also remove the need to use a semaphore to protect the control (though you may still need one to protect a pending URL list depending on what you do about the first point). (this is already done, sorry, missed it).

  • What is the purpose of the 1000 millisecond sleep? it appears before the tasks are even queued.

\$\endgroup\$
3
  • \$\begingroup\$ thanks for the tips. the semaphore is used to allow only 10 max concurrent downloads. when i click start, the button doesn't get disabled immediately, so the 1000ms sleep, although it makes no difference, i'll remove it. if each threads perform the GetHtml, is there need for asynchronously? concerning threads management, am using .net 2.0. which way would i do this? \$\endgroup\$
    – Smith
    Jun 27, 2011 at 12:30
  • \$\begingroup\$ @Smith: Sorry, I miss-read some of your code and have now updated my answer. \$\endgroup\$ Jun 27, 2011 at 13:35
  • \$\begingroup\$ If you use the asynchronous methods, you shouldn't create tasks for the thread queue. The asynchronous method call already does this for you. Ultimately, the goal of the two suggestions is to throttle the web requests before the thread pool thread is needlessly consumed. \$\endgroup\$ Jun 27, 2011 at 13:35

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.