HTTPDownloader: Improve error reporting

Give something human-readable when an error occurs.

src/common/error.h

@@ -141,6 +141,6 @@ class Error
   void AddPrefixFmtArgs(fmt::string_view fmt, fmt::format_args args);
   void AddSuffixFmtArgs(fmt::string_view fmt, fmt::format_args args);
 
-  Type m_type = Type::None;
   std::string m_description;
+  Type m_type = Type::None;
 };

src/core/achievements.cpp

@@ -354,10 +354,13 @@ std::string Achievements::GetLocalImagePath(const std::string_view image_name, i
 
 void Achievements::DownloadImage(std::string url, std::string cache_filename)
 {
-  auto callback = [cache_filename](s32 status_code, const std::string& content_type,
+  auto callback = [cache_filename](s32 status_code, const Error& error, const std::string& content_type,
                                    HTTPDownloader::Request::Data data) {
     if (status_code != HTTPDownloader::HTTP_STATUS_OK)
+    {
+      ERROR_LOG("Failed to download badge '{}': {}", Path::GetFileName(cache_filename), error.GetDescription());
       return;
+    }
 
     if (!FileSystem::WriteBinaryFile(cache_filename.c_str(), data.data(), data.size()))
     {
@@ -753,18 +756,22 @@ uint32_t Achievements::ClientReadMemory(uint32_t address, uint8_t* buffer, uint3
 void Achievements::ClientServerCall(const rc_api_request_t* request, rc_client_server_callback_t callback,
                                     void* callback_data, rc_client_t* client)
 {
-  HTTPDownloader::Request::Callback hd_callback =
-    [callback, callback_data](s32 status_code, const std::string& content_type, HTTPDownloader::Request::Data data) {
-      rc_api_server_response_t rr;
-      rr.http_status_code = (status_code <= 0) ? (status_code == HTTPDownloader::HTTP_STATUS_CANCELLED ?
-                                                    RC_API_SERVER_RESPONSE_CLIENT_ERROR :
-                                                    RC_API_SERVER_RESPONSE_RETRYABLE_CLIENT_ERROR) :
-                                                 status_code;
-      rr.body_length = data.size();
-      rr.body = reinterpret_cast<const char*>(data.data());
-
-      callback(&rr, callback_data);
-    };
+  HTTPDownloader::Request::Callback hd_callback = [callback, callback_data](s32 status_code, const Error& error,
+                                                                            const std::string& content_type,
+                                                                            HTTPDownloader::Request::Data data) {
+    if (status_code != HTTPDownloader::HTTP_STATUS_OK)
+      ERROR_LOG("Server call failed: {}", error.GetDescription());
+
+    rc_api_server_response_t rr;
+    rr.http_status_code = (status_code <= 0) ? (status_code == HTTPDownloader::HTTP_STATUS_CANCELLED ?
+                                                  RC_API_SERVER_RESPONSE_CLIENT_ERROR :
+                                                  RC_API_SERVER_RESPONSE_RETRYABLE_CLIENT_ERROR) :
+                                               status_code;
+    rr.body_length = data.size();
+    rr.body = reinterpret_cast<const char*>(data.data());
+
+    callback(&rr, callback_data);
+  };
 
   HTTPDownloader* http = static_cast<HTTPDownloader*>(rc_client_get_userdata(client));
 

src/core/game_list.cpp

@@ -1454,10 +1454,11 @@ bool GameList::DownloadCovers(const std::vector<std::string>& url_templates, boo
     return false;
   }
 
-  std::unique_ptr<HTTPDownloader> downloader(HTTPDownloader::Create(Host::GetHTTPUserAgent()));
+  Error error;
+  std::unique_ptr<HTTPDownloader> downloader(HTTPDownloader::Create(Host::GetHTTPUserAgent(), &error));
   if (!downloader)
   {
-    progress->DisplayError("Failed to create HTTP downloader.");
+    progress->DisplayError(fmt::format("Failed to create HTTP downloader:\n{}", error.GetDescription()));
     return false;
   }
 
@@ -1484,39 +1485,43 @@ bool GameList::DownloadCovers(const std::vector<std::string>& url_templates, boo
 
     // we could actually do a few in parallel here...
     std::string filename = Path::URLDecode(url);
-    downloader->CreateRequest(
-      std::move(url), [use_serial, &save_callback, entry_path = std::move(entry_path), filename = std::move(filename)](
-                        s32 status_code, const std::string& content_type, HTTPDownloader::Request::Data data) {
-        if (status_code != HTTPDownloader::HTTP_STATUS_OK || data.empty())
-          return;
-
-        std::unique_lock lock(s_mutex);
-        const GameList::Entry* entry = GetEntryForPath(entry_path);
-        if (!entry || !GetCoverImagePathForEntry(entry).empty())
-          return;
-
-        // prefer the content type from the response for the extension
-        // otherwise, if it's missing, and the request didn't have an extension.. fall back to jpegs.
-        std::string template_filename;
-        std::string content_type_extension(HTTPDownloader::GetExtensionForContentType(content_type));
-
-        // don't treat the domain name as an extension..
-        const std::string::size_type last_slash = filename.find('/');
-        const std::string::size_type last_dot = filename.find('.');
-        if (!content_type_extension.empty())
-          template_filename = fmt::format("cover.{}", content_type_extension);
-        else if (last_slash != std::string::npos && last_dot != std::string::npos && last_dot > last_slash)
-          template_filename = Path::GetFileName(filename);
-        else
-          template_filename = "cover.jpg";
-
-        std::string write_path(GetNewCoverImagePathForEntry(entry, template_filename.c_str(), use_serial));
-        if (write_path.empty())
-          return;
-
-        if (FileSystem::WriteBinaryFile(write_path.c_str(), data.data(), data.size()) && save_callback)
-          save_callback(entry, std::move(write_path));
-      });
+    downloader->CreateRequest(std::move(url), [use_serial, &save_callback, entry_path = std::move(entry_path),
+                                               filename = std::move(filename)](s32 status_code, const Error& error,
+                                                                               const std::string& content_type,
+                                                                               HTTPDownloader::Request::Data data) {
+      if (status_code != HTTPDownloader::HTTP_STATUS_OK || data.empty())
+      {
+        ERROR_LOG("Download for {} failed: {}", Path::GetFileName(filename), error.GetDescription());
+        return;
+      }
+
+      std::unique_lock lock(s_mutex);
+      const GameList::Entry* entry = GetEntryForPath(entry_path);
+      if (!entry || !GetCoverImagePathForEntry(entry).empty())
+        return;
+
+      // prefer the content type from the response for the extension
+      // otherwise, if it's missing, and the request didn't have an extension.. fall back to jpegs.
+      std::string template_filename;
+      std::string content_type_extension(HTTPDownloader::GetExtensionForContentType(content_type));
+
+      // don't treat the domain name as an extension..
+      const std::string::size_type last_slash = filename.find('/');
+      const std::string::size_type last_dot = filename.find('.');
+      if (!content_type_extension.empty())
+        template_filename = fmt::format("cover.{}", content_type_extension);
+      else if (last_slash != std::string::npos && last_dot != std::string::npos && last_dot > last_slash)
+        template_filename = Path::GetFileName(filename);
+      else
+        template_filename = "cover.jpg";
+
+      std::string write_path(GetNewCoverImagePathForEntry(entry, template_filename.c_str(), use_serial));
+      if (write_path.empty())
+        return;
+
+      if (FileSystem::WriteBinaryFile(write_path.c_str(), data.data(), data.size()) && save_callback)
+        save_callback(entry, std::move(write_path));
+    });
     downloader->WaitForAllRequests();
     progress->IncrementProgressValue();
   }

src/duckstation-qt/autoupdaterdialog.cpp

@@ -88,9 +88,10 @@ AutoUpdaterDialog::AutoUpdaterDialog(QWidget* parent /* = nullptr */) : QDialog(
   connect(m_ui.skipThisUpdate, &QPushButton::clicked, this, &AutoUpdaterDialog::skipThisUpdateClicked);
   connect(m_ui.remindMeLater, &QPushButton::clicked, this, &AutoUpdaterDialog::remindMeLaterClicked);
 
-  m_http = HTTPDownloader::Create(Host::GetHTTPUserAgent());
+  Error error;
+  m_http = HTTPDownloader::Create(Host::GetHTTPUserAgent(), &error);
   if (!m_http)
-    ERROR_LOG("Failed to create HTTP downloader, auto updater will not be available.");
+    ERROR_LOG("Failed to create HTTP downloader, auto updater will not be available:\n{}", error.GetDescription());
 }
 
 AutoUpdaterDialog::~AutoUpdaterDialog() = default;
@@ -277,7 +278,7 @@ void AutoUpdaterDialog::queueUpdateCheck(bool display_message)
   }
 
   m_http->CreateRequest(LATEST_TAG_URL, std::bind(&AutoUpdaterDialog::getLatestTagComplete, this, std::placeholders::_1,
-                                                  std::placeholders::_3));
+                                                  std::placeholders::_2, std::placeholders::_4));
 #else
   emit updateCheckCompleted();
 #endif
@@ -294,11 +295,11 @@ void AutoUpdaterDialog::queueGetLatestRelease()
 
   std::string url = fmt::format(fmt::runtime(LATEST_RELEASE_URL), getCurrentUpdateTag());
   m_http->CreateRequest(std::move(url), std::bind(&AutoUpdaterDialog::getLatestReleaseComplete, this,
-                                                  std::placeholders::_1, std::placeholders::_3));
+                                                  std::placeholders::_1, std::placeholders::_2, std::placeholders::_4));
 #endif
 }
 
-void AutoUpdaterDialog::getLatestTagComplete(s32 status_code, std::vector<u8> response)
+void AutoUpdaterDialog::getLatestTagComplete(s32 status_code, const Error& error, std::vector<u8> response)
 {
 #ifdef UPDATE_CHECKER_SUPPORTED
   const std::string selected_tag(getCurrentUpdateTag());
@@ -351,14 +352,14 @@ void AutoUpdaterDialog::getLatestTagComplete(s32 status_code, std::vector<u8> re
   else
   {
     if (m_display_messages)
-      reportError(fmt::format("Failed to download latest tag info: HTTP {}", status_code));
+      reportError(fmt::format("Failed to download latest tag info: {}", error.GetDescription()));
   }
 
   emit updateCheckCompleted();
 #endif
 }
 
-void AutoUpdaterDialog::getLatestReleaseComplete(s32 status_code, std::vector<u8> response)
+void AutoUpdaterDialog::getLatestReleaseComplete(s32 status_code, const Error& error, std::vector<u8> response)
 {
 #ifdef UPDATE_CHECKER_SUPPORTED
   if (status_code == HTTPDownloader::HTTP_STATUS_OK)
@@ -415,7 +416,7 @@ void AutoUpdaterDialog::getLatestReleaseComplete(s32 status_code, std::vector<u8
   }
   else
   {
-    reportError(fmt::format("Failed to download latest release info: HTTP {}", status_code));
+    reportError(fmt::format("Failed to download latest release info: {}", error.GetDescription()));
   }
 
   emit updateCheckCompleted();
@@ -430,11 +431,11 @@ void AutoUpdaterDialog::queueGetChanges()
 
   std::string url = fmt::format(fmt::runtime(CHANGES_URL), g_scm_hash_str, getCurrentUpdateTag());
   m_http->CreateRequest(std::move(url), std::bind(&AutoUpdaterDialog::getChangesComplete, this, std::placeholders::_1,
-                                                  std::placeholders::_3));
+                                                  std::placeholders::_2, std::placeholders::_4));
 #endif
 }
 
-void AutoUpdaterDialog::getChangesComplete(s32 status_code, std::vector<u8> response)
+void AutoUpdaterDialog::getChangesComplete(s32 status_code, const Error& error, std::vector<u8> response)
 {
 #ifdef UPDATE_CHECKER_SUPPORTED
   if (status_code == HTTPDownloader::HTTP_STATUS_OK)
@@ -503,7 +504,7 @@ void AutoUpdaterDialog::getChangesComplete(s32 status_code, std::vector<u8> resp
   }
   else
   {
-    reportError(fmt::format("Failed to download change list: HTTP {}", status_code));
+    reportError(fmt::format("Failed to download change list: {}", error.GetDescription()));
   }
 #endif
 }
@@ -527,13 +528,13 @@ void AutoUpdaterDialog::downloadUpdateClicked()
 
   m_http->CreateRequest(
     m_download_url.toStdString(),
-    [this, &download_result](s32 status_code, const std::string&, std::vector<u8> response) {
+    [this, &download_result](s32 status_code, const Error& error, const std::string&, std::vector<u8> response) {
       if (status_code == HTTPDownloader::HTTP_STATUS_CANCELLED)
         return;
 
       if (status_code != HTTPDownloader::HTTP_STATUS_OK)
       {
-        reportError(fmt::format("Download failed: HTTP status code {}", status_code));
+        reportError(fmt::format("Download failed: {}", error.GetDescription()));
         download_result = false;
         return;
       }

src/duckstation-qt/autoupdaterdialog.h

@@ -58,11 +58,11 @@ private Q_SLOTS:
   bool updateNeeded() const;
   std::string getCurrentUpdateTag() const;
 
-  void getLatestTagComplete(s32 status_code, std::vector<u8> response);
-  void getLatestReleaseComplete(s32 status_code, std::vector<u8> response);
+  void getLatestTagComplete(s32 status_code, const Error& error, std::vector<u8> response);
+  void getLatestReleaseComplete(s32 status_code, const Error& error, std::vector<u8> response);
 
   void queueGetChanges();
-  void getChangesComplete(s32 status_code, std::vector<u8> response);
+  void getChangesComplete(s32 status_code, const Error& error, std::vector<u8> response);
 
   bool processUpdate(const std::vector<u8>& update_data);
 

src/duckstation-qt/qthost.cpp

@@ -261,11 +261,13 @@ std::optional<bool> QtHost::DownloadFile(QWidget* parent, const QString& title,
 {
   static constexpr u32 HTTP_POLL_INTERVAL = 10;
 
-  std::unique_ptr<HTTPDownloader> http = HTTPDownloader::Create(Host::GetHTTPUserAgent());
+  Error error;
+  std::unique_ptr<HTTPDownloader> http = HTTPDownloader::Create(Host::GetHTTPUserAgent(), &error);
   if (!http)
   {
     QMessageBox::critical(parent, qApp->translate("QtHost", "Error"),
-                          qApp->translate("QtHost", "Failed to create HTTPDownloader."));
+                          qApp->translate("QtHost", "Failed to create HTTPDownloader:\n%1")
+                            .arg(QString::fromStdString(error.GetDescription())));
     return false;
   }
 
@@ -281,14 +283,16 @@ std::optional<bool> QtHost::DownloadFile(QWidget* parent, const QString& title,
 
   http->CreateRequest(
     std::move(url),
-    [parent, data, &download_result](s32 status_code, const std::string&, std::vector<u8> hdata) {
+    [parent, data, &download_result](s32 status_code, const Error& error, const std::string&, std::vector<u8> hdata) {
       if (status_code == HTTPDownloader::HTTP_STATUS_CANCELLED)
         return;
 
       if (status_code != HTTPDownloader::HTTP_STATUS_OK)
       {
         QMessageBox::critical(parent, qApp->translate("QtHost", "Error"),
-                              qApp->translate("QtHost", "Download failed with HTTP status code %1.").arg(status_code));
+                              qApp->translate("QtHost", "Download failed with HTTP status code %1:\n%2")
+                                .arg(status_code)
+                                .arg(QString::fromStdString(error.GetDescription())));
         download_result = false;
         return;
       }
@@ -1913,11 +1917,13 @@ std::string Host::GetClipboardText()
 {
   // Hope this doesn't deadlock...
   std::string ret;
-  QtHost::RunOnUIThread([&ret]() {
-    QClipboard* clipboard = QGuiApplication::clipboard();
-    if (clipboard)
-      ret = clipboard->text().toStdString();
-  }, true);
+  QtHost::RunOnUIThread(
+    [&ret]() {
+      QClipboard* clipboard = QGuiApplication::clipboard();
+      if (clipboard)
+        ret = clipboard->text().toStdString();
+    },
+    true);
   return ret;
 }
 

src/util/CMakeLists.txt

@@ -224,7 +224,6 @@ if(WIN32)
     dinput_source.cpp
     dinput_source.h
     http_downloader_winhttp.cpp
-    http_downloader_winhttp.h
     platform_misc_win32.cpp
     win32_raw_input_source.cpp
     win32_raw_input_source.h
@@ -275,7 +274,6 @@ endif()
 if(NOT WIN32 AND NOT ANDROID)
   target_sources(util PRIVATE
     http_downloader_curl.cpp
-    http_downloader_curl.h
   )
   target_link_libraries(util PRIVATE
     CURL::libcurl

src/util/http_downloader.cpp

@@ -109,7 +109,8 @@ void HTTPDownloader::LockedPollRequests(std::unique_lock<std::mutex>& lock)
       m_pending_http_requests.erase(m_pending_http_requests.begin() + index);
       lock.unlock();
 
-      req->callback(HTTP_STATUS_TIMEOUT, std::string(), Request::Data());
+      req->error.SetStringFmt("Request timed out after {} seconds.", m_timeout);
+      req->callback(HTTP_STATUS_TIMEOUT, req->error, std::string(), Request::Data());
 
       CloseRequest(req);
 
@@ -126,7 +127,8 @@ void HTTPDownloader::LockedPollRequests(std::unique_lock<std::mutex>& lock)
       m_pending_http_requests.erase(m_pending_http_requests.begin() + index);
       lock.unlock();
 
-      req->callback(HTTP_STATUS_CANCELLED, std::string(), Request::Data());
+      req->error.SetStringView("Request was cancelled.");
+      req->callback(HTTP_STATUS_CANCELLED, req->error, std::string(), Request::Data());
 
       CloseRequest(req);
 
@@ -159,7 +161,9 @@ void HTTPDownloader::LockedPollRequests(std::unique_lock<std::mutex>& lock)
 
     // run callback with lock unheld
     lock.unlock();
-    req->callback(req->status_code, req->content_type, std::move(req->data));
+    if (req->status_code != HTTP_STATUS_OK)
+      req->error.SetStringFmt("Request failed with HTTP status code {}", req->status_code);
+    req->callback(req->status_code, req->error, req->content_type, std::move(req->data));
     CloseRequest(req);
     lock.lock();
   }