WebClient asynchronous downloader not working properly

Question

So, I've been hammering away at this for some time now. I'm an amateur programmer, and so I don't always know what I'm doing wrong.

Anyhow, the premise of my recent project:

My friends and I regularly play MineCraft, but they're not terribly bright, and we're not always around to get the mods and send them links and whatnot. So I thought I'd program something to automatically pull down mods so they are synchronized with the server and get the server data at the same time.

I'm using a free FTP host, but I don't think that's the issue here, for reasons that will become clear.

Basically, I want to make use of a progressbar and ideally a label too in order to indicate the progress of the overall block of data (all mods together... not more than 1GB - quite a bit smaller). However, I seem to be running into a few issues regarding the Async option:

• It will randomly opt to not download files it should be downloading

• It may not download files in their entirety before claiming to be complete

• The progressbar is maybe 50% full when the msgbox triggers saying that it is finished downloading all items.

However, while the progressbar doesn't work due to the reporting events for progress not existing in the synchronous use of Webclient, when I run the syncro in a BGworker, it downloads properly every time. However, I lose out on progress reporting, which is kind of important....

So, basically:

• Is there a better way to implement this?

This is the last chunk I need to get working before it's ready to go, so I'd really like to try and do that. Thank you for any assistance!

Edit: Updated with code:

Public Function GetDownloadSize(ByVal URL As String) As Long
    Dim request As Net.FtpWebRequest = DirectCast(Net.WebRequest.Create(URL), Net.FtpWebRequest)
    request.Method = Net.WebRequestMethods.Ftp.GetFileSize
    request.Credentials = New Net.NetworkCredential(dl_user, dl_pass)
    Dim response As Net.FtpWebResponse = DirectCast(request.GetResponse(), Net.FtpWebResponse)
    Dim fileSize As Long = response.ContentLength
    Return fileSize
End Function

Private Sub btn_sync_Click(sender As Object, e As EventArgs) Handles btn_sync.Click
    Dim cont As DialogResult = MsgBox("Continue? " + (total_dl_size / 1000).ToString("N0") + " KB remain to be downloaded.", MsgBoxStyle.YesNo, "CAUTION!")
    If cont = DialogResult.No Then
        tb_warnings.AppendText("-ERR: User declined to synchronize files. Restart the application to sync.")
        tb_warnings.AppendText(ControlChars.NewLine)
        Label3.BackColor = Color.Firebrick
        Return
    End If
    btn_sync.Enabled = False
    btn_scan.Enabled = false
    tb_warnings.AppendText("-Deleting outmoded/unused mods. Protected mods will be kept.")
    For Each i As fdata_obj In deleted_files
        My.Computer.FileSystem.DeleteFile(mc_dir + "\mods\" + i.name)
    Next
    tb_warnings.AppendText(ControlChars.NewLine)
    tb_warnings.AppendText("-Deleting mod subdirectories to ensure no conflicts.")
    tb_warnings.AppendText(ControlChars.NewLine)

    For Each d In My.Computer.FileSystem.GetDirectories(mc_dir + "\mods")
        My.Computer.FileSystem.DeleteDirectory(d, FileIO.DeleteDirectoryOption.DeleteAllContents)
    Next

    initialize_download()


End Sub

Private Sub initialize_download()

           Dim wc As New System.Net.WebClient() ' SORRY, ASSUME THIS IS A PUBLIC VAR SO IT CAN BE REFERENCED ACROSS ITS OTHER METHODS
    AddHandler wc.DownloadProgressChanged, AddressOf OnDownloadProgressChanged
    AddHandler wc, AddressOf OnFileDownloadCompleted

    Dim usr As String = "randouser"
    Dim pass As String = "randopass"
    For Each s In (From dl As fdata_obj In new_files Select dl_server + "/mods/" + mods_dir + "/" + dl.name).ToList
        downloads.Enqueue(s)
    Next
    wc.Credentials = New Net.NetworkCredential(usr, pass)

        Dim urix As String = downloads.Dequeue
        Try
            wc.DownloadFileasync(New Uri(urix), mc_dir + "\mods\" + IO.Path.GetFileName(urix))
        Catch ex As Exception
            MsgBox(ex.Message)
            If tb_warnings.InvokeRequired = True Then
                tb_warnings.Invoke(New tb_updater(AddressOf tb_update), "-ERR: Could not download file: " + urix, urix)
            Else
                tb_warnings.AppendText("-ERR: Could not download file: " + IO.Path.GetFileName(urix))
                tb_warnings.AppendText(ControlChars.NewLine)

            End If
    end try
End Sub
Private Sub OnDownloadProgressChanged(ByVal sender As Object, ByVal e As System.Net.DownloadProgressChangedEventArgs)
    MsgBox("This is happening!")
    total_dl = total_dl + e.BytesReceived
    Dim percentage As Integer = (CType((total_dl / total_dl_size), Integer) * 100)
    if percentage > 100 then
        percentage = 100
    endif
    prog_update(percentage)

End Sub

delegate sub progress_update(byval prog as integer)
' POTENTIAL ISSUES HERE???????
private sub prog_update(byval prog as integer)
    if progressbar1.invokerequired then
        progressbar1.invoke(new prog_update(addressof progress),prog)
    else
        progressbar1.value = prog


Private Sub OnFileDownloadCompleted(ByVal sender As Net.WebClient, ByVal e As System.ComponentModel.AsyncCompletedEventArgs)

    If e.Cancelled Then
        MsgBox(e.Cancelled)
    ElseIf Not e.Error Is Nothing Then
        MsgBox(e.Error.Message)
    Else
    if downloads.count > 0 then
                Dim urix As String = downloads.Dequeue
        Try
            wc.DownloadFileasync(New Uri(urix), mc_dir + "\mods\" + IO.Path.GetFileName(urix))
        Catch ex As Exception
            MsgBox(ex.Message)
            If tb_warnings.InvokeRequired = True Then
                tb_warnings.Invoke(New tb_updater(AddressOf tb_update), "-ERR: Could not download file: " + urix, urix)
            Else
                tb_warnings.AppendText("-ERR: Could not download file: " + IO.Path.GetFileName(urix))
                tb_warnings.AppendText(ControlChars.NewLine)

            End If
        End Try
    End If

End Sub

Answer 1

First of all, the main reason your progress bar isn't working is because of this:

Dim percentage As Integer = (CType((total_dl / total_dl_size), Integer) * 100)

The code will first evaluate total_dl / total_dl_size, say it results in 0.34, then it will convert that into an integer which will result in 0 (0.34 is rounded down to zero, since integers have no decimals), and finally it multiplies that 0 with 100 (which still results in 0).
What you've got to do is to multiply the dividend with 100 first so that the result will go from 0-100 instead of 0-1: (total_dl * 100) / total_dl_size.

As for thread-safety (invoking) I always use this extension method that I created:

Imports System.Runtime.CompilerServices

Public Module Extensions
    <Extension()> _
    Public Sub InvokeIfRequired(ByVal Control As Control, ByVal Method As [Delegate], ByVal ParamArray Parameters As Object())
        If Parameters Is Nothing OrElse _
            Parameters.Length = 0 Then Parameters = Nothing 'If Parameters is null or has a length of zero then no parameters should be passed.
        If Control.InvokeRequired = True Then
            Control.Invoke(Method, Parameters)
        Else
            Method.DynamicInvoke(Parameters)
        End If
    End Sub
End Module

(preferrably put it in another file)

That, together with Lambda expressions (introduced in Visual Studio 2010), will greatly simplify invocation for you. This is because instead of putting the If InvokeRequired pattern everywhere:

If Me.InvokeRequired Then
    Me.Invoke(New Action(AddressOf SomeMethod), params)
Else
    SomeMethod()
End If

you only need to type:

Me.InvokeIfRequired(AddressOf SomeMethod, params)

and the extension method will do the rest for you.

And if you use lambda expressions you can create methods dynamically:

Me.InvokeIfRequired(Sub()
                        Label1.Text = "Hello world!"
                        ProgressBar1.Value += 1
                    End Sub)

---

Now, to your code.

I have separated your code a bit more so that it's easier to deal with. For starters, instead of copy-pasting the download code to the DownloadFileCompleted event handler I made a more generic method called DownloadFile().

''' <summary>
''' Downloads a file from the specified URL with the specified credentials.
''' </summary>
''' <param name="URL">The URL of the file.</param>
''' <param name="Username">The username which to login with.</param>
''' <param name="Password">The password which to login with.</param>
''' <remarks></remarks>
Private Sub DownloadFile(ByVal URL As String, ByVal Username As String, ByVal Password As String)
    If wc.IsBusy = True Then Throw New Exception("A download is already ongoing!")

    wc.Credentials = New NetworkCredential(dl_user, dl_pass)
    total_dl_size = GetDownloadSize(URL, Username, Password)

    Try
        Dim FileName As String = Path.GetFileName(URL)
        AppendWarning("Downloading " & FileName & "...")
        wc.DownloadFileAsync(New Uri(URL), Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Desktop), FileName))
    Catch ex As Exception
        AppendWarning("-ERR: Could not download file: " & Path.GetFileName(URL))
    End Try
End Sub

As you see I also made a generic method for outputting warnings and error messages:

''' <summary>
''' (Thread-safe) Appends a warning or status message to the "tb_warnings" text box.
''' </summary>
''' <param name="Text">The text to append.</param>
''' <remarks></remarks>
Private Sub AppendWarning(ByVal Text As String)
    Me.InvokeIfRequired(Sub() tb_warnings.AppendText(Text & Environment.NewLine))
End Sub

Here's the entire code, it works properly for me:

Private dl_user As String = "someusername"
Private dl_pass As String = "somepassword"

Private dl_urls As String() = {"URL1", "URL2"} 'Temporary. Use your own code.
Private total_dl_size As Long = 0
Private total_dl As Long = 0

Dim WithEvents wc As New System.Net.WebClient()
Dim downloads As New Queue(Of String)

Private Sub Form1_Load(sender As System.Object, e As System.EventArgs) Handles MyBase.Load
    'Populate the download queue.
    downloads.Enqueue(dl_urls(0)) 'Temporary. Use your own code here.
    downloads.Enqueue(dl_urls(1))
End Sub

'The download button.
Private Sub Button1_Click(sender As System.Object, e As System.EventArgs) Handles Button1.Click
    'Do your pre-download stuff here.

    DownloadFile(downloads.Dequeue(), dl_user, dl_pass) 'Download the first file.
End Sub

''' <summary>
''' Downloads a file from the specified URL with the specified credentials.
''' </summary>
''' <param name="URL">The URL of the file.</param>
''' <param name="Username">The username which to login with.</param>
''' <param name="Password">The password which to login with.</param>
''' <remarks></remarks>
Private Sub DownloadFile(ByVal URL As String, ByVal Username As String, ByVal Password As String)
    If wc.IsBusy = True Then Throw New Exception("A download is already ongoing!")

    wc.Credentials = New NetworkCredential(dl_user, dl_pass) 'Set the credentials.
    total_dl_size = GetDownloadSize(URL, Username, Password) 'Get the size of the current file.

    Try
        Dim FileName As String = Path.GetFileName(URL) 'Get the current file's name.
        AppendWarning("Downloading " & FileName & "...") 'Download notice.
        wc.DownloadFileAsync(New Uri(URL), Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Desktop), FileName)) 'Download the file to the desktop (use your own path here).
    Catch ex As Exception
        AppendWarning("-ERR: Could not download file: " & Path.GetFileName(URL))
    End Try
End Sub

''' <summary>
''' (Thread-safe) Appends a warning or status message to the "tb_warnings" text box.
''' </summary>
''' <param name="Text">The text to append.</param>
''' <remarks></remarks>
Private Sub AppendWarning(ByVal Text As String)
    Me.InvokeIfRequired(Sub() tb_warnings.AppendText(Text & Environment.NewLine))
End Sub

Private Sub wc_DownloadProgressChanged(sender As Object, e As System.Net.DownloadProgressChangedEventArgs) Handles wc.DownloadProgressChanged
    Me.InvokeIfRequired(Sub()
                            Dim Progress As Integer = CType(Math.Round((e.BytesReceived * 100) / total_dl_size), Integer)
                            If Progress > 100 Then Progress = 100
                            If Progress < 0 Then Progress = 0
                            ProgressBar1.Value = Progress
                        End Sub)
End Sub

Private Sub wc_DownloadFileCompleted(sender As Object, e As System.ComponentModel.AsyncCompletedEventArgs) Handles wc.DownloadFileCompleted
    If e.Cancelled Then
        MessageBox.Show(e.Cancelled)

    ElseIf Not e.Error Is Nothing Then
        MessageBox.Show(e.Error.Message)

    Else
        If downloads.Count > 0 Then
            DownloadFile(downloads.Dequeue(), dl_user, dl_pass) 'Download the next file.
        Else
            AppendWarning("Download complete!")
        End If

    End If
End Sub

Some other things to keep in mind:

• The MsgBox() function exists purely for backwards compatibility. You should use .NET's standard MessageBox.Show() method instead.

• String concatenation should be done using the ampersand (&) instead of the plus (+). See why.

• Concatenating paths should always be done using Path.Combine() as it will ensure to create a proper path. If you input anything invalid it'll throw an exception.

  Usage:

  Path.Combine(Path1, Path2, Path3, ...)
  Path.Combine("C:\", "Foo") 'Results in: C:\Foo
  Path.Combine("C:\", "Foo", "Bar", "Hello World.txt") 'Results in: C:\Foo\Bar\Hello World.txt
  

Hope this helps!