Cleanup Download code, fixing SDL_Thread leaking

The download thread was setting itself to nullptr (d->mThread = nullptr)
in a number of locations. This caused a later call to SDL_WaitThread to
be unable to perform cleanup.

This reverts most of 1eb02f83a5d3895e4e18db30ea10d88da94ba4c0 (including
making Download::cancel no longer blocking), but keeps the necessary
waiting for the thread to finish before freeing the memory buffer in
~UpdaterWindow(), which might have been the bug fixed by that change.

Fixed removal of downloaded .part file when its checksum failed. It
trying to remove the file without .part appended instead.

Fixed download progress indication in ServerDialog to not be reversed,
though this is rarely visible due to the server list being so small.

Fixed reporting of curl error.

6 files changed, 179 insertions, 245 deletions

diff --git a/src/gui/serverdialog.cpp b/src/gui/serverdialog.cpp
index 01b477da..90469662 100644
--- a/src/gui/serverdialog.cpp
+++ b/src/gui/serverdialog.cpp
@@ -50,8 +50,6 @@
 #include <cstdlib>
 #include <string>
 
-static const int MAX_SERVERLIST = 6;
-
 ServersListModel::ServersListModel(ServerInfos *servers, ServerDialog *parent):
         mServers(servers),
         mVersionStrings(servers->size(), VersionString(0, std::string())),
@@ -141,7 +139,6 @@ public:
             if (info.version.first > 0)
             {
                 graphics->setColor(unsupported);
-
                 graphics->drawText(info.version.second,
                                    getWidth() - info.version.first - 2, top);
             }
@@ -231,9 +228,11 @@ ServerDialog::~ServerDialog()
     if (mDownload)
     {
         mDownload->cancel();
-        delete mDownload;
-        mDownload = nullptr;
+
+        // Make sure thread is gone before deleting the ServersListModel
+        mDownload.reset();
     }
+
     delete mServersListModel;
 }
 
@@ -262,6 +261,7 @@ void ServerDialog::action(const gcn::ActionEvent &event)
         else
         {
             mDownload->cancel();
+
             mQuitButton->setEnabled(false);
             mConnectButton->setEnabled(false);
             mDeleteButton->setEnabled(false);
@@ -374,7 +374,7 @@ void ServerDialog::logic()
         else if (mDownloadStatus == DOWNLOADING_IN_PROGRESS)
         {
             mDownloadText->setCaption(strprintf(_("Downloading server list..."
-                                                 "%2.2f%%"),
+                                                 "%2.0f%%"),
                                       mDownloadProgress * 100));
         }
         else if (mDownloadStatus == DOWNLOADING_IDLE)
@@ -406,7 +406,8 @@ void ServerDialog::downloadServerList()
     if (listFile.empty())
         listFile = "https://www.manasource.org/serverlist.xml";
 
-    mDownload = new Net::Download(this, listFile, &downloadUpdate);
+    mDownload = std::make_unique<Net::Download>(this, listFile,
+                                                &ServerDialog::downloadUpdate);
     mDownload->setFile(mDir + "/serverlist.xml");
     mDownload->start();
 }
@@ -494,7 +495,6 @@ void ServerDialog::loadServers()
         server.version.first = gui->getFont()->getWidth(version);
         server.version.second = version;
 
-        MutexLocker lock(&mMutex);
         // Add the server to the local list if it's not already present
         bool found = false;
         int i = 0;
@@ -562,17 +562,18 @@ void ServerDialog::saveCustomServers(const ServerInfo &currentServer, int index)
 }
 
 int ServerDialog::downloadUpdate(void *ptr, DownloadStatus status,
-                                 size_t total, size_t remaining)
+                                 size_t dltotal, size_t dlnow)
 {
     if (status == DOWNLOAD_STATUS_CANCELLED)
         return -1;
 
     auto *sd = reinterpret_cast<ServerDialog*>(ptr);
-    bool finished = false;
+    MutexLocker lock(&sd->mMutex);
 
     if (status == DOWNLOAD_STATUS_COMPLETE)
     {
-        finished = true;
+        sd->loadServers();
+        sd->mDownloadStatus = DOWNLOADING_COMPLETE;
     }
     else if (status < 0)
     {
@@ -582,27 +583,13 @@ int ServerDialog::downloadUpdate(void *ptr, DownloadStatus status,
     }
     else
     {
-        float progress = (float) remaining / total;
+        float progress = 0.0f;
+        if (dltotal > 0)
+            progress = static_cast<float>(dlnow) / dltotal;
 
-        if (progress != progress)
-            progress = 0.0f; // check for NaN
-        else if (progress < 0.0f)
-            progress = 0.0f; // no idea how this could ever happen, but why not check for it anyway.
-        else if (progress > 1.0f)
-            progress = 1.0f;
-
-        MutexLocker lock(&sd->mMutex);
         sd->mDownloadStatus = DOWNLOADING_IN_PROGRESS;
         sd->mDownloadProgress = progress;
     }
 
-    if (finished)
-    {
-        sd->loadServers();
-
-        MutexLocker lock(&sd->mMutex);
-        sd->mDownloadStatus = DOWNLOADING_COMPLETE;
-    }
-
     return 0;
 }
diff --git a/src/gui/serverdialog.h b/src/gui/serverdialog.h
index b61689bd..bf2a9512 100644
--- a/src/gui/serverdialog.h
+++ b/src/gui/serverdialog.h
@@ -33,6 +33,7 @@
 #include <guichan/listmodel.hpp>
 #include <guichan/selectionlistener.hpp>
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -131,7 +132,7 @@ class ServerDialog : public Window,
         void loadCustomServers();
 
         static int downloadUpdate(void *ptr, DownloadStatus status,
-                                  size_t total, size_t remaining);
+                                  size_t dltotal, size_t dlnow);
 
         Label  *mDescription;
         Button *mQuitButton;
@@ -158,11 +159,11 @@ class ServerDialog : public Window,
         /** Status of the current download. */
         ServerDialogDownloadStatus mDownloadStatus = DOWNLOADING_PREPARING;
 
-        Net::Download *mDownload = nullptr;
+        std::unique_ptr<Net::Download> mDownload;
         Label *mDownloadText;
 
         Mutex mMutex;
-        float mDownloadProgress = -1.0f;
+        float mDownloadProgress = 0.0f;
 
         ServerInfos mServers;
         ServerInfo *mServerInfo;
diff --git a/src/gui/updaterwindow.cpp b/src/gui/updaterwindow.cpp
index bd98637b..852d3ade 100644
--- a/src/gui/updaterwindow.cpp
+++ b/src/gui/updaterwindow.cpp
@@ -46,8 +46,8 @@
 #include <iostream>
 #include <fstream>
 
-const std::string xmlUpdateFile = "resources.xml";
-const std::string txtUpdateFile = "resources2.txt";
+constexpr char xmlUpdateFile[] = "resources.xml";
+constexpr char txtUpdateFile[] = "resources2.txt";
 
 /**
  * Load the given file into a vector of updateFiles.
@@ -174,9 +174,10 @@ UpdaterWindow::~UpdaterWindow()
     {
         mDownload->cancel();
 
-        delete mDownload;
-        mDownload = nullptr;
+        // Make sure thread is gone before freeing the memory buffer
+        mDownload.reset();
     }
+
     free(mMemoryBuffer);
 }
 
@@ -251,19 +252,8 @@ void UpdaterWindow::loadNews()
         return;
     }
 
-    // Reallocate and include terminating 0 character
-    mMemoryBuffer = (char*)realloc(mMemoryBuffer, mDownloadedBytes + 1);
-    mMemoryBuffer[mDownloadedBytes] = '\0';
-
     mBrowserBox->clearRows();
-
-    // Tokenize and add each line separately
-    char *line = strtok(mMemoryBuffer, "\n");
-    while (line)
-    {
-        mBrowserBox->addRow(line);
-        line = strtok(nullptr, "\n");
-    }
+    mBrowserBox->addRows(std::string_view(mMemoryBuffer, mDownloadedBytes));
 
     // Free the memory buffer now that we don't need it anymore
     free(mMemoryBuffer);
@@ -273,7 +263,7 @@ void UpdaterWindow::loadNews()
 }
 
 int UpdaterWindow::updateProgress(void *ptr, DownloadStatus status,
-                                  size_t dt, size_t dn)
+                                  size_t dltotal, size_t dlnow)
 {
     auto *uw = reinterpret_cast<UpdaterWindow *>(ptr);
 
@@ -287,20 +277,15 @@ int UpdaterWindow::updateProgress(void *ptr, DownloadStatus status,
         uw->mDownloadStatus = UPDATE_ERROR;
     }
 
-    float progress = (float) dn / dt;
-
-    if (progress != progress)
-        progress = 0.0f; // check for NaN
-    if (progress < 0.0f)
-        progress = 0.0f; // no idea how this could ever happen, but why not check for it anyway.
-    if (progress > 1.0f)
-        progress = 1.0f;
+    float progress = 0.0f;
+    if (dltotal > 0)
+        progress = static_cast<float>(dlnow) / dltotal;
 
     uw->setLabel(
             uw->mCurrentFile + " (" + toString((int) (progress * 100)) + "%)");
     uw->setProgress(progress);
 
-    if (Client::getState() != STATE_UPDATE || uw->mDownloadStatus == UPDATE_ERROR)
+    if (Client::getState() != STATE_UPDATE)
     {
         // If the action was canceled return an error code to stop the mThread
         return -1;
@@ -309,15 +294,15 @@ int UpdaterWindow::updateProgress(void *ptr, DownloadStatus status,
     return 0;
 }
 
-size_t UpdaterWindow::memoryWrite(void *ptr, size_t size, size_t nmemb, void *stream)
+size_t UpdaterWindow::memoryWrite(char *ptr, size_t size, size_t nmemb, void *stream)
 {
     auto *uw = reinterpret_cast<UpdaterWindow *>(stream);
-    size_t totalMem = size * nmemb;
+    const size_t totalMem = size * nmemb;
     uw->mMemoryBuffer = (char*) realloc(uw->mMemoryBuffer,
                                         uw->mDownloadedBytes + totalMem);
     if (uw->mMemoryBuffer)
     {
-        memcpy(&(uw->mMemoryBuffer[uw->mDownloadedBytes]), ptr, totalMem);
+        memcpy(uw->mMemoryBuffer + uw->mDownloadedBytes, ptr, totalMem);
         uw->mDownloadedBytes += totalMem;
     }
 
@@ -326,8 +311,9 @@ size_t UpdaterWindow::memoryWrite(void *ptr, size_t size, size_t nmemb, void *st
 
 void UpdaterWindow::download()
 {
-    mDownload = new Net::Download(this, mUpdateHost + "/" + mCurrentFile,
-                                  updateProgress);
+    mDownload = std::make_unique<Net::Download>(this,
+                                                mUpdateHost + "/" + mCurrentFile,
+                                                &UpdaterWindow::updateProgress);
 
     if (mStoreInMemory)
     {
@@ -335,15 +321,11 @@ void UpdaterWindow::download()
     }
     else
     {
+        std::optional<unsigned long> adler32;
         if (mDownloadStatus == UPDATE_RESOURCES)
-        {
-            mDownload->setFile(mUpdatesDir + "/" + mCurrentFile,
-                               mCurrentChecksum);
-        }
-        else
-        {
-            mDownload->setFile(mUpdatesDir + "/" + mCurrentFile);
-        }
+            adler32 = mCurrentChecksum;
+
+        mDownload->setFile(mUpdatesDir + "/" + mCurrentFile, adler32);
     }
 
     if (mDownloadStatus != UPDATE_RESOURCES)
@@ -365,8 +347,8 @@ void UpdaterWindow::loadUpdates()
         if (mUpdateFiles.empty())
         {
             logger->log("Warning this server does not have a"
-                        " %s file falling back to %s", xmlUpdateFile.c_str(),
-                        txtUpdateFile.c_str());
+                        " %s file falling back to %s", xmlUpdateFile,
+                        txtUpdateFile);
             mUpdateFiles = loadTxtFile(mUpdatesDir + "/" + txtUpdateFile);
         }
     }
@@ -398,20 +380,19 @@ void UpdaterWindow::logic()
 
     switch (mDownloadStatus)
     {
-        case UPDATE_ERROR:
-            // TODO: Only send complete sentences to gettext
-            mBrowserBox->addRow(std::string());
-            mBrowserBox->addRow(_("##1  The update process is incomplete."));
-            // TRANSLATORS: Continues "you try again later.".
-            mBrowserBox->addRow(_("##1  It is strongly recommended that"));
-            // TRANSLATORS: Begins "It is strongly recommended that".
-            mBrowserBox->addRow(_("##1  you try again later."));
-
-            mBrowserBox->addRow(mDownload->getError());
+        case UPDATE_ERROR: {
+            std::string error = "##1";
+            error += mDownload->getError();
+            error += "\n\n";
+            error += _("The update process is incomplete. "
+                       "It is strongly recommended that you try again later.");
+            mBrowserBox->addRows(error);
+
             mScrollArea->setVerticalScrollAmount(
                     mScrollArea->getVerticalMaxScroll());
             mDownloadStatus = UPDATE_COMPLETE;
             break;
+        }
         case UPDATE_NEWS:
             if (mDownloadComplete)
             {
diff --git a/src/gui/updaterwindow.h b/src/gui/updaterwindow.h
index 65ee1d88..5c749e18 100644
--- a/src/gui/updaterwindow.h
+++ b/src/gui/updaterwindow.h
@@ -113,12 +113,12 @@ private:
      * A download callback for progress updates.
      */
     static int updateProgress(void *ptr, DownloadStatus status,
-                              size_t dt, size_t dn);
+                              size_t dltotal, size_t dlnow);
 
     /**
      * A libcurl callback for writing to memory.
      */
-    static size_t memoryWrite(void *ptr, size_t size, size_t nmemb,
+    static size_t memoryWrite(char *ptr, size_t size, size_t nmemb,
                               void *stream);
 
     enum UpdateDownloadStatus
@@ -165,13 +165,13 @@ private:
     bool mUserCancel = false;
 
     /** Byte count currently downloaded in mMemoryBuffer. */
-    int mDownloadedBytes = 0;
+    size_t mDownloadedBytes = 0;
 
     /** Buffer for files downloaded to memory. */
     char *mMemoryBuffer = nullptr;
 
     /** Download handle. */
-    Net::Download *mDownload = nullptr;
+    std::unique_ptr<Net::Download> mDownload;
 
     /** List of files to download. */
     std::vector<UpdateFile> mUpdateFiles;
diff --git a/src/net/download.cpp b/src/net/download.cpp
index 8a41ebfa..2cfdc3a1 100644
--- a/src/net/download.cpp
+++ b/src/net/download.cpp
@@ -31,7 +31,7 @@
 
 #include <zlib.h>
 
-const char *DOWNLOAD_ERROR_MESSAGE_THREAD = "Could not create download thread!";
+constexpr char DOWNLOAD_ERROR_MESSAGE_THREAD[] = "Could not create download thread!";
 
 namespace Net {
 
@@ -59,12 +59,12 @@ unsigned long Download::fadler32(FILE *file)
     return adler;
 }
 
-
-Download::Download(void *ptr, const std::string &url,
-                   DownloadUpdate updateFunction):
-        mPtr(ptr),
-        mUrl(url),
-        mUpdateFunction(updateFunction)
+Download::Download(void *ptr,
+                   const std::string &url,
+                   DownloadUpdate updateFunction)
+    : mPtr(ptr)
+    , mUrl(url)
+    , mUpdateFunction(updateFunction)
 {
     mError = (char*) malloc(CURL_ERROR_SIZE);
     mError[0] = 0;
@@ -74,17 +74,15 @@ Download::Download(void *ptr, const std::string &url,
 
 Download::~Download()
 {
-    if (mHeaders)
-        curl_slist_free_all(mHeaders);
+    SDL_WaitThread(mThread, nullptr);
 
-    int status;
-    SDL_WaitThread(mThread, &status);
+    curl_slist_free_all(mHeaders);
     free(mError);
 }
 
-void Download::addHeader(const std::string &header)
+void Download::addHeader(const char *header)
 {
-    mHeaders = curl_slist_append(mHeaders, header.c_str());
+    mHeaders = curl_slist_append(mHeaders, header);
 }
 
 void Download::noCache()
@@ -101,7 +99,7 @@ void Download::setFile(const std::string &filename,
     mAdler = adler32;
 }
 
-void Download::setWriteFunction(WriteFunction write)
+void Download::setWriteFunction(curl_write_callback write)
 {
     mOptions.memoryWrite = true;
     mWriteFunction = write;
@@ -116,7 +114,7 @@ bool Download::start()
     if (!mThread)
     {
         logger->log("%s", DOWNLOAD_ERROR_MESSAGE_THREAD);
-        strcpy(mError, DOWNLOAD_ERROR_MESSAGE_THREAD);
+        strncpy(mError, DOWNLOAD_ERROR_MESSAGE_THREAD, CURL_ERROR_SIZE - 1);
         mUpdateFunction(mPtr, DOWNLOAD_STATUS_THREAD_ERROR, 0, 0);
 
         return false;
@@ -128,13 +126,7 @@ bool Download::start()
 void Download::cancel()
 {
     logger->log("Canceling download: %s", mUrl.c_str());
-
     mOptions.cancel = true;
-    if (mThread && SDL_GetThreadID(mThread) != 0)
-    {
-        SDL_WaitThread(mThread, nullptr);
-        mThread = nullptr;
-    }
 }
 
 const char *Download::getError() const
@@ -147,177 +139,149 @@ int Download::downloadProgress(void *clientp,
                                curl_off_t ultotal, curl_off_t ulnow)
 {
     auto *d = reinterpret_cast<Download*>(clientp);
+    DownloadStatus status = d->mOptions.cancel ? DOWNLOAD_STATUS_CANCELLED
+                                               : DOWNLOAD_STATUS_IN_PROGRESS;
 
-    if (d->mOptions.cancel)
-    {
-        return d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_CANCELLED,
-                                  (size_t) dltotal, (size_t) dlnow);
-        return -5;
-    }
-
-    return d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_IDLE,
-                              (size_t) dltotal, (size_t) dlnow);
+    return d->mUpdateFunction(d->mPtr, status, (size_t) dltotal, (size_t) dlnow);
 }
 
 int Download::downloadThread(void *ptr)
 {
-    int attempts = 0;
-    bool complete = false;
     auto *d = reinterpret_cast<Download*>(ptr);
-    CURLcode res;
+    bool complete = false;
     std::string outFilename;
 
-    if (!d)
-    {
-        return 0;
-    }
     if (!d->mOptions.memoryWrite)
-    {
         outFilename = d->mFileName + ".part";
-    }
 
-    while (attempts < 3 && !complete && !d->mOptions.cancel)
+    for (int attempts = 0; attempts < 3 && !complete && !d->mOptions.cancel; ++attempts)
     {
-        FILE *file = nullptr;
-
         d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_STARTING, 0, 0);
 
-        if (d->mOptions.cancel)
-        {
-            d->mThread = nullptr;
-            return 0;
-        }
+        CURL *curl = curl_easy_init();
+        if (!curl)
+            break;
+
+        logger->log("Downloading: %s", d->mUrl.c_str());
 
-        d->mCurl = curl_easy_init();
+        curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
+        curl_easy_setopt(curl, CURLOPT_HTTPHEADER, d->mHeaders);
 
-        if (d->mCurl && !d->mOptions.cancel)
+        FILE *file = nullptr;
+
+        if (d->mOptions.memoryWrite)
+        {
+            curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1);
+            curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, d->mWriteFunction);
+            curl_easy_setopt(curl, CURLOPT_WRITEDATA, d->mPtr);
+        }
+        else
         {
-            logger->log("Downloading: %s", d->mUrl.c_str());
+            file = fopen(outFilename.c_str(), "w+b");
+            curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
+        }
 
-            curl_easy_setopt(d->mCurl, CURLOPT_FOLLOWLOCATION, 1);
-            curl_easy_setopt(d->mCurl, CURLOPT_HTTPHEADER, d->mHeaders);
+        const std::string appShort = branding.getStringValue("appShort");
+        const std::string userAgent =
+                strprintf(PACKAGE_EXTENDED_VERSION, appShort.c_str());
 
-            if (d->mOptions.memoryWrite)
-            {
-                curl_easy_setopt(d->mCurl, CURLOPT_FAILONERROR, 1);
-                curl_easy_setopt(d->mCurl, CURLOPT_WRITEFUNCTION, d->mWriteFunction);
-                curl_easy_setopt(d->mCurl, CURLOPT_WRITEDATA, d->mPtr);
-            }
-            else
-            {
-                file = fopen(outFilename.c_str(), "w+b");
-                curl_easy_setopt(d->mCurl, CURLOPT_WRITEDATA, file);
-            }
+        curl_easy_setopt(curl, CURLOPT_USERAGENT, userAgent.c_str());
+        curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, d->mError);
+        curl_easy_setopt(curl, CURLOPT_URL, d->mUrl.c_str());
+        curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0);
+        curl_easy_setopt(curl, CURLOPT_XFERINFOFUNCTION, downloadProgress);
+        curl_easy_setopt(curl, CURLOPT_XFERINFODATA, ptr);
+        curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1);
+        curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 15);
 
-            const std::string appShort = branding.getStringValue("appShort");
-            const std::string userAgent =
-                    strprintf(PACKAGE_EXTENDED_VERSION, appShort.c_str());
+        const CURLcode res = curl_easy_perform(curl);
+        curl_easy_cleanup(curl);
 
-            curl_easy_setopt(d->mCurl, CURLOPT_USERAGENT, userAgent.c_str());
-            curl_easy_setopt(d->mCurl, CURLOPT_ERRORBUFFER, d->mError);
-            curl_easy_setopt(d->mCurl, CURLOPT_URL, d->mUrl.c_str());
-            curl_easy_setopt(d->mCurl, CURLOPT_NOPROGRESS, 0);
-            curl_easy_setopt(d->mCurl, CURLOPT_XFERINFOFUNCTION, downloadProgress);
-            curl_easy_setopt(d->mCurl, CURLOPT_XFERINFODATA, ptr);
-            curl_easy_setopt(d->mCurl, CURLOPT_NOSIGNAL, 1);
-            curl_easy_setopt(d->mCurl, CURLOPT_CONNECTTIMEOUT, 15);
+        if (res == CURLE_ABORTED_BY_CALLBACK)
+        {
+            d->mOptions.cancel = true;
 
-            if ((res = curl_easy_perform(d->mCurl)) != 0 && !d->mOptions.cancel)
+            if (file)
             {
-                switch (res)
-                {
-                case CURLE_ABORTED_BY_CALLBACK:
-                    d->mOptions.cancel = true;
-                    break;
-                case CURLE_COULDNT_CONNECT:
-                default:
-                    logger->log("curl error %d: %s host: %s",
-                                res, d->mError, d->mUrl.c_str());
-                    break;
-                }
+                fclose(file);
+                ::remove(outFilename.c_str());
+            }
 
-                if (d->mOptions.cancel)
-                {
-                    break;
-                }
+            break;
+        }
 
-                d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_ERROR, 0, 0);
+        if (res != CURLE_OK)
+        {
+            logger->log("curl error %d: %s host: %s",
+                        res, d->mError, d->mUrl.c_str());
 
-                if (!d->mOptions.memoryWrite)
-                {
-                    fclose(file);
-                    ::remove(outFilename.c_str());
-                }
-                attempts++;
-                continue;
+            if (file)
+            {
+                fclose(file);
+                ::remove(outFilename.c_str());
             }
 
-            curl_easy_cleanup(d->mCurl);
+            break;
+        }
 
-            if (!d->mOptions.memoryWrite)
+        if (!d->mOptions.memoryWrite)
+        {
+            // Don't check resources.xml checksum
+            if (d->mAdler)
             {
-                // Don't check resources.xml checksum
-                if (d->mAdler)
-                {
-                    unsigned long adler = fadler32(file);
+                unsigned long adler = fadler32(file);
 
-                    if (d->mAdler != adler)
-                    {
+                if (d->mAdler != adler)
+                {
+                    if (file)
                         fclose(file);
 
-                        // Remove the corrupted file
-                        ::remove(d->mFileName.c_str());
-                        logger->log("Checksum for file %s failed: (%lx/%lx)",
-                            d->mFileName.c_str(),
-                            adler, *d->mAdler);
-                        attempts++;
-                        continue; // Bail out here to avoid the renaming
-                    }
+                    // Remove the corrupted file
+                    ::remove(outFilename.c_str());
+                    logger->log("Checksum for file %s failed: (%lx/%lx)",
+                        d->mFileName.c_str(),
+                        adler, *d->mAdler);
+
+                    continue; // Bail out here to avoid the renaming
                 }
+            }
+
+            if (file)
                 fclose(file);
 
-                // Any existing file with this name is deleted first, otherwise
-                // the rename will fail on Windows.
-                ::remove(d->mFileName.c_str());
-                ::rename(outFilename.c_str(), d->mFileName.c_str());
+            // Any existing file with this name is deleted first, otherwise
+            // the rename will fail on Windows.
+            ::remove(d->mFileName.c_str());
+            ::rename(outFilename.c_str(), d->mFileName.c_str());
 
-                // Check if we can open it and no errors were encountered
-                // during renaming
-                file = fopen(d->mFileName.c_str(), "rb");
-                if (file)
-                {
-                    fclose(file);
-                    complete = true;
-                }
-            }
-            else
+            // Check if we can open it and no errors were encountered
+            // during renaming
+            file = fopen(d->mFileName.c_str(), "rb");
+            if (file)
             {
-                // It's stored in memory, we're done
+                fclose(file);
+                file = nullptr;
                 complete = true;
             }
         }
-        if (d->mOptions.cancel)
+        else
         {
-            d->mThread = nullptr;
-            return 0;
+            // It's stored in memory, we're done
+            complete = true;
         }
-        attempts++;
-    }
 
-    if (d->mOptions.cancel)
-    {
-        // Nothing to do...
+        if (file)
+            fclose(file);
     }
-    else if (!complete || attempts >= 3)
-    {
-        d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_ERROR, 0, 0);
-    }
-    else
+
+    if (!d->mOptions.cancel)
     {
-        d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_COMPLETE, 0, 0);
+        if (complete)
+            d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_COMPLETE, 0, 0);
+        else
+            d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_ERROR, 0, 0);
     }
 
-    d->mThread = nullptr;
     return 0;
 }
 
diff --git a/src/net/download.h b/src/net/download.h
index b0f79e79..be2e374f 100644
--- a/src/net/download.h
+++ b/src/net/download.h
@@ -18,7 +18,6 @@
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <cstdlib> // pulls in int64_t
 #include <cstdio>
 #include <string>
 #include <optional>
@@ -33,28 +32,31 @@ enum DownloadStatus
     DOWNLOAD_STATUS_THREAD_ERROR = -2,
     DOWNLOAD_STATUS_ERROR = -1,
     DOWNLOAD_STATUS_STARTING = 0,
-    DOWNLOAD_STATUS_IDLE,
+    DOWNLOAD_STATUS_IN_PROGRESS,
     DOWNLOAD_STATUS_COMPLETE
 };
 
-using DownloadUpdate = int (*)(void *, DownloadStatus, size_t, size_t);
-
-// Matches what CURL expects
-using WriteFunction = size_t (*)(void *, size_t, size_t, void *);
-
 struct SDL_Thread;
-using CURL = void;
-struct curl_slist;
 
 namespace Net {
+
 class Download
 {
     public:
-        Download(void *ptr, const std::string &url, DownloadUpdate updateFunction);
+        /**
+         * Callback function for download updates.
+         *
+         * @param ptr       Pointer passed to Download constructor
+         * @param status    Current download status
+         * @param dltotal   Total number of bytes to download
+         * @param dlnow     Number of bytes downloaded so far
+         */
+        using DownloadUpdate = int (*)(void *, DownloadStatus, size_t, size_t);
 
+        Download(void *ptr, const std::string &url, DownloadUpdate updateFunction);
         ~Download();
 
-        void addHeader(const std::string &header);
+        void addHeader(const char *header);
 
         /**
          * Convience method for adding no-cache headers.
@@ -64,7 +66,7 @@ class Download
         void setFile(const std::string &filename,
                      std::optional<unsigned long> adler32 = {});
 
-        void setWriteFunction(WriteFunction write);
+        void setWriteFunction(curl_write_callback write);
 
         /**
          * Starts the download thread.
@@ -76,7 +78,7 @@ class Download
 
         /**
          * Cancels the download. Returns immediately, the cancelled status will
-         * be noted in the next avialable update call.
+         * be noted in the next available update call.
          */
         void cancel();
 
@@ -96,11 +98,10 @@ class Download
             unsigned memoryWrite: 1;
         } mOptions;
         std::string mFileName;
-        WriteFunction mWriteFunction = nullptr;
+        curl_write_callback mWriteFunction = nullptr;
         std::optional<unsigned long> mAdler;
         DownloadUpdate mUpdateFunction;
         SDL_Thread *mThread = nullptr;
-        CURL *mCurl = nullptr;
         curl_slist *mHeaders = nullptr;
         char *mError;
 };