Version check: bound the UI-thread download with an overall timeout

SerialPrograms/Source/CommonFramework/Startup/NewVersionCheck.cpp

@@ -204,9 +204,14 @@ void check_new_version(Logger& logger){
     logger.log("Checking for new version...");
     JsonValue json;
     try{
+        //  This runs on the UI thread (panel switching / on_idle), so the
+        //  request gets a hard deadline: setTransferTimeout() only covers
+        //  data transfer, and a connection that stalls would otherwise
+        //  freeze the UI until the OS gives up on the TCP attempt.
         json = FileDownloader::download_json_file(
             logger,
-            "https://raw.githubusercontent.com/PokemonAutomation/ComputerControl/master/LatestVersion.json"
+            "https://raw.githubusercontent.com/PokemonAutomation/ComputerControl/master/LatestVersion.json",
+            std::chrono::seconds(3)
         );
     }catch (OperationFailedException&){
         return;

SerialPrograms/Source/CommonFramework/Tools/FileDownloader.cpp

@@ -27,7 +27,7 @@
 namespace PokemonAutomation{
 namespace FileDownloader{
 
-std::string download_file(Logger& logger, const std::string& url){
+std::string download_file(Logger& logger, const std::string& url, std::chrono::seconds overall_timeout){
 //    cout << "download_file()" << endl;
     QNetworkAccessManager network_access_manager;
     QByteArray downloaded_data;
@@ -57,6 +57,19 @@ std::string download_file(Logger& logger, const std::string& url){
     request.setTransferTimeout(std::chrono::seconds(5));
     reply.reset(network_access_manager.get(request));
 
+    //  Deadline for the whole request, including the connection phase
+    //  (setTransferTimeout() above only limits data transfer). Without
+    //  this, a connection that stalls would block loop.exec() forever.
+    QTimer overall_timer;
+    if (overall_timeout.count() > 0){
+        overall_timer.setSingleShot(true);
+        QObject::connect(
+            &overall_timer, &QTimer::timeout,
+            &loop, [&loop]{ loop.exit(); }
+        );
+        overall_timer.start((int)overall_timeout.count());
+    }
+
 #if 0
     QTimer timer;
     timer.singleShot(
@@ -72,11 +85,18 @@ std::string download_file(Logger& logger, const std::string& url){
 //    cout << "loop.exec() - enter" << endl;
     loop.exec();
 //    cout << "loop.exec() - exit" << endl;
+    overall_timer.stop();
 
     if (!reply){
         std::string str = "QNetworkReply is null.";
 //        logger.log(str, COLOR_RED);
         throw_and_log<OperationFailedException>(logger, ErrorReportMode::NO_ERROR_REPORT, str);
+    }else if (overall_timeout.count() > 0 && reply->isRunning()){
+        //  The overall deadline elapsed while the request (typically the
+        //  connection) was still in progress.
+        reply->abort();
+        throw_and_log<OperationFailedException>(logger, ErrorReportMode::NO_ERROR_REPORT,
+            "Timed out after " + std::to_string(overall_timeout.count()) + " seconds downloading " + url + ".");
     }else if (reply->error() == QNetworkReply::NoError){
 //        QString contents = QString::fromUtf8(reply->readAll());
 //        qDebug() << contents;
@@ -179,8 +199,8 @@ void download_file_to_disk(
     reply->deleteLater();
 }
 
-JsonValue download_json_file(Logger& logger, const std::string& url){
-    std::string downloaded_data = download_file(logger, url);
+JsonValue download_json_file(Logger& logger, const std::string& url, std::chrono::seconds overall_timeout){
+    std::string downloaded_data = download_file(logger, url, overall_timeout);
     return parse_json(downloaded_data);
 }
 

SerialPrograms/Source/CommonFramework/Tools/FileDownloader.h

@@ -8,6 +8,7 @@
 #define PokemonAutomation_FileDownloader_H
 
 #include <string>
+#include <chrono>
 #include <functional>
 #include "Common/Cpp/CancellableScope.h"
 
@@ -18,7 +19,16 @@ namespace PokemonAutomation{
 namespace FileDownloader{
 
 //  Throws OperationFailedException if failed to download.
-std::string download_file(Logger& logger, const std::string& url);
+//  overall_timeout is a deadline for the whole request including the
+//  connection phase (QNetworkRequest::setTransferTimeout() only limits
+//  data transfer, so without it a connection that stalls would block
+//  this call forever). std::chrono::seconds(0) (the default) imposes no
+//  overall limit.
+std::string download_file(
+    Logger& logger,
+    const std::string& url,
+    std::chrono::seconds overall_timeout = std::chrono::seconds(0)
+);
 
 //  Throws OperationFailedException if failed to download.
 void download_file_to_disk(
@@ -32,7 +42,11 @@ void download_file_to_disk(
 
 //  Throws OperationFailedException if failed to download.
 //  Returns empty value if invalid JSON.
-JsonValue download_json_file(Logger& logger, const std::string& url);
+JsonValue download_json_file(
+    Logger& logger,
+    const std::string& url,
+    std::chrono::seconds overall_timeout = std::chrono::seconds(0)
+);
 
 
 }