Merge #796

796: Fulfil promise for restart error condition r=townsend2010 a=gerboland

Fixes #764. We were omitting to set a promised future, which caused the client to hang.

I've added a basic test to catch this behaviour for this command. Later PR will try to generalise it for the other commands.

Co-authored-by: Gerry Boland <[email protected]>

Author: bors[bot]

src/daemon/daemon.cpp

@@ -1493,18 +1493,22 @@ try // clang-format on
     const auto& instances = instances_and_status.first; // use structured bindings instead in C++17
     auto& status = instances_and_status.second;         // idem
 
-    if (status.ok())
+    if (!status.ok())
     {
-        status = cmd_vms(instances,
-                         std::bind(&Daemon::reboot_vm, this, std::placeholders::_1)); // 1st pass to reboot all targets
+        return status_promise->set_value(status);
+    }
 
-        if (status.ok())
-        {
-            auto future_watcher = create_future_watcher();
-            future_watcher->setFuture(QtConcurrent::run(this, &Daemon::async_wait_for_ready_all<RestartReply>, server,
-                                                        instances, status_promise));
-        }
+    status = cmd_vms(instances,
+                     std::bind(&Daemon::reboot_vm, this, std::placeholders::_1)); // 1st pass to reboot all targets
+
+    if (!status.ok())
+    {
+        return status_promise->set_value(status);
     }
+
+    auto future_watcher = create_future_watcher();
+    future_watcher->setFuture(
+        QtConcurrent::run(this, &Daemon::async_wait_for_ready_all<RestartReply>, server, instances, status_promise));
 }
 catch (const std::exception& e)
 {
@@ -1756,7 +1760,7 @@ void mp::Daemon::persist_instances()
     mp::write_json(instance_records_json, data_dir.filePath(instance_db_name));
 }
 
-void mp::Daemon::start_mount(VirtualMachine *vm, const std::string& name, const std::string& source_path,
+void mp::Daemon::start_mount(VirtualMachine* vm, const std::string& name, const std::string& source_path,
                              const std::string& target_path, const std::unordered_map<int, int>& gid_map,
                              const std::unordered_map<int, int>& uid_map)
 {
@@ -2058,7 +2062,7 @@ grpc::Status mp::Daemon::cmd_vms(const std::vector<std::string>& tgts, std::func
     return grpc::Status::OK;
 }
 
-void mp::Daemon::install_sshfs(VirtualMachine *vm, const std::string& name)
+void mp::Daemon::install_sshfs(VirtualMachine* vm, const std::string& name)
 {
     auto& key_provider = *config->ssh_key_provider;
 

tests/test_daemon.cpp

@@ -58,6 +58,10 @@ using namespace testing;
 
 namespace
 {
+template<typename R>
+  bool is_ready(std::future<R> const& f)
+  { return f.wait_for(std::chrono::seconds(0)) == std::future_status::ready; }
+
 struct MockDaemon : public mp::Daemon
 {
     using mp::Daemon::Daemon;
@@ -336,6 +340,21 @@ TEST_F(Daemon, provides_version)
     EXPECT_THAT(stream.str(), HasSubstr(mp::version_string));
 }
 
+TEST_F(Daemon, failed_restart_command_returns_fulfilled_promise)
+{
+    mp::Daemon daemon{config_builder.build()};
+
+    auto nonexistant_instance = new mp::InstanceNames; // on heap as *Request takes ownership
+    nonexistant_instance->add_instance_name("nonexistant");
+    mp::RestartRequest request;
+    request.set_allocated_instance_names(nonexistant_instance);
+    std::promise<grpc::Status> status_promise;
+
+    daemon.restart(&request, nullptr, &status_promise);
+    EXPECT_TRUE(is_ready(status_promise.get_future()));
+}
+
+
 namespace
 {
 struct DaemonCreateLaunchTestSuite : public Daemon, public WithParamInterface<std::string>