Keep UploadFinished Paired with UploadStarted When an Upload Throws

The exception from a throwing upload was caught around perform_job, so
the UploadFinished lifecycle event was skipped and plugins saw an
upload start that never finished.

The catch now sits around the upload call. The error is reported
through the job's error callback and UploadFinished is fired with an
error code, as for any other failed upload. The worker keeps running
for the next job. The started, upload and finished sequence moved to
PrintHostJobQueue::upload_job so it can be tested without the dialog.

src/slic3r/Utils/PrintHost.cpp

@@ -320,12 +320,7 @@ void PrintHostJobQueue::priv::bg_thread_main()
                 % job.cancelled;
 
             if (! job.cancelled) {
-                // A failing job must not stop the worker, or later jobs would stay queued forever.
+                perform_job(std::move(job));
-                try {
-                    perform_job(std::move(job));
-                } catch (const std::exception &e) {
-                    emit_error(e.what());
-                }
             }
 
             remove_source();
@@ -437,12 +432,10 @@ void PrintHostJobQueue::priv::remove_source()
     source_to_remove.clear();
 }
 
-void PrintHostJobQueue::priv::perform_job(PrintHostJob the_job)
+bool PrintHostJobQueue::upload_job(PrintHostJob &job, PrintHost::ProgressFn progress_fn, PrintHost::ErrorFn error_fn, PrintHost::InfoFn info_fn)
 {
-    emit_progress(0);   // Indicate the upload is starting
-
     // Captured before upload_data is moved into upload() below.
-    const std::string upload_filename = the_job.upload_data.source_path.filename().string();
+    const std::string upload_filename = job.upload_data.source_path.filename().string();
 
     {
         LifecycleEventContext ctx;
@@ -451,19 +444,37 @@ void PrintHostJobQueue::priv::perform_job(PrintHostJob the_job)
         fire_lifecycle_event(LifecycleEvent::UploadStarted, ctx);
     }
 
-    bool success = the_job.printhost->upload(std::move(the_job.upload_data),
+    bool success = false;
-        [this](Http::Progress progress, bool &cancel)   { this->progress_fn(std::move(progress), cancel); },
+    std::string error;
-        [this](wxString error)                          { this->error_fn(std::move(error)); },
+    // A throwing upload must not stop the worker, or later jobs would stay queued forever.
-        [this](wxString tag, wxString host)             { this->info_fn(std::move(tag), std::move(host)); }
+    try {
-    );
+        success = job.printhost->upload(std::move(job.upload_data), std::move(progress_fn), error_fn, std::move(info_fn));
+    } catch (const std::exception &e) {
+        error = e.what();
+        error_fn(error);
+    }
 
     {
         LifecycleEventContext ctx;
         ctx.name  = upload_filename;
         ctx.code  = success ? LifecycleEvtCode::Ok : LifecycleEvtCode::Error;
+        ctx.msg   = error;
         fire_lifecycle_event(LifecycleEvent::UploadFinished, ctx);
     }
 
+    return success;
+}
+
+void PrintHostJobQueue::priv::perform_job(PrintHostJob the_job)
+{
+    emit_progress(0);   // Indicate the upload is starting
+
+    bool success = PrintHostJobQueue::upload_job(the_job,
+        [this](Http::Progress progress, bool &cancel)   { this->progress_fn(std::move(progress), cancel); },
+        [this](wxString error)                          { this->error_fn(std::move(error)); },
+        [this](wxString tag, wxString host)             { this->info_fn(std::move(tag), std::move(host)); }
+    );
+
     if (success) {
         emit_progress(100);
         if (the_job.switch_to_device_tab) {

src/slic3r/Utils/PrintHost.hpp

@@ -152,6 +152,10 @@ public:
     void enqueue(PrintHostJob job);
     void cancel(size_t id);
 
+    // Uploads the job, firing UploadStarted and a matching UploadFinished. An exception thrown by
+    // the upload is reported through error_fn and makes the upload fail.
+    static bool upload_job(PrintHostJob &job, PrintHost::ProgressFn progress_fn, PrintHost::ErrorFn error_fn, PrintHost::InfoFn info_fn);
+
 private:
     struct priv;
     std::shared_ptr<priv> p;

tests/slic3rutils/test_printhost.cpp

@@ -1,10 +1,13 @@
 #include <catch2/catch_all.hpp>
 
+#include <memory>
+#include <stdexcept>
 #include <string>
 #include <vector>
 
 #include <nlohmann/json.hpp>
 
+#include "libslic3r/LifecycleEvents.hpp"
 #include "slic3r/Utils/PrintHost.hpp"
 #include "slic3r/Utils/Flashforge.hpp"
 
@@ -28,6 +31,41 @@ public:
     std::string get_host() const override { return {}; }
 };
 
+class ThrowingPrintHost : public TestPrintHost
+{
+public:
+    bool upload(PrintHostUpload, ProgressFn, ErrorFn, InfoFn) const override { throw std::runtime_error("reply could not be read"); }
+};
+
+struct UploadEvents
+{
+    std::vector<LifecycleEvent>   events;
+    std::vector<LifecycleEvtCode> codes;
+    std::vector<std::string>      errors;
+    bool                          uploaded{false};
+
+    explicit UploadEvents(std::unique_ptr<PrintHost> host)
+    {
+        set_lifecycle_hook_fn([this](LifecycleEvent event, const LifecycleEventContext& ctx) {
+            events.push_back(event);
+            codes.push_back(ctx.code);
+        });
+
+        PrintHostJob job;
+        job.printhost               = std::move(host);
+        job.upload_data.source_path = "plate.gcode";
+        try {
+            uploaded = PrintHostJobQueue::upload_job(job, [](Http::Progress, bool&) {},
+                                                     [this](wxString error) { errors.push_back(error.ToStdString()); },
+                                                     [](wxString, wxString) {});
+        } catch (...) {
+            set_lifecycle_hook_fn(nullptr);
+            throw;
+        }
+        set_lifecycle_hook_fn(nullptr);
+    }
+};
+
 std::string format_error(const std::string& body, const std::string& error, unsigned status)
 {
     return TestPrintHost().format_error(body, error, status).ToStdString();
@@ -314,3 +352,23 @@ TEST_CASE("Flashforge material slots reject a reply that is not JSON", "[PrintHo
     CHECK_FALSE(ok);
     CHECK(slots.empty());
 }
+
+TEST_CASE("An upload that throws still finishes with an error", "[PrintHost][LifecycleEvents]")
+{
+    UploadEvents run(std::make_unique<ThrowingPrintHost>());
+
+    CHECK_FALSE(run.uploaded);
+    CHECK(run.errors == std::vector<std::string>{"reply could not be read"});
+    CHECK(run.events == std::vector<LifecycleEvent>{LifecycleEvent::UploadStarted, LifecycleEvent::UploadFinished});
+    CHECK(run.codes == std::vector<LifecycleEvtCode>{LifecycleEvtCode::Ok, LifecycleEvtCode::Error});
+}
+
+TEST_CASE("A successful upload finishes without an error", "[PrintHost][LifecycleEvents]")
+{
+    UploadEvents run(std::make_unique<TestPrintHost>());
+
+    CHECK(run.uploaded);
+    CHECK(run.errors.empty());
+    CHECK(run.events == std::vector<LifecycleEvent>{LifecycleEvent::UploadStarted, LifecycleEvent::UploadFinished});
+    CHECK(run.codes == std::vector<LifecycleEvtCode>{LifecycleEvtCode::Ok, LifecycleEvtCode::Ok});
+}