Bugfix: Do not block in record processing (fixes #5).

With #4, a bug was introduced that causes processing of the bo record
triggering the execution of the command to block until the execution of
the command complete, when the no-wait flag is set.

The cause was that Command::run created a future using std::async, but
this future was destroyed when the function returned. However, the
destructor of std::future may block when the future has been created by
std::async and the shared state is not available yet.

This is fixed now by keeping the future and only destroying it on the
next call to Command::run, when the shared state is available.

Author: smarsching

executeApp/src/Command.cpp

@@ -30,7 +30,6 @@
 #include <algorithm>
 #include <cassert>
 #include <cstring>
-#include <future>
 #include <stdexcept>
 #include <system_error>
 #include <utility>
@@ -628,6 +627,15 @@ void Command::run() {
     stdinBuffer = this->stdinBuffer;
     stderrCapacity = this->stderrCapacity;
     stdoutCapacity = this->stdoutCapacity;
+    // We want to clean up futures that have been created by earlier calls to
+    // this method and that are ready now. We cannot remove futures that are not
+    // ready yet, because this operation might block.
+    pendingFutures.remove_if([](std::future<void> const &future) {
+      return (
+        !future.valid()
+        || (future.wait_for(std::chrono::seconds::zero())
+          != std::future_status::timeout));
+    });
   }
   // The pointers stored in the following two vectors are only valid as long as
   // the original vectors exist and have not been changed. This is okay, because
@@ -790,7 +798,22 @@ void Command::run() {
       // process to terminate, we have to do this in the spawned thread.
       // Otherwise, the child process would never be reaped after terminating
       // and stay as a zombie process until the whole IOC terminates.
-      stdinPipe.writeDataAsyncAndWaitForPid(childPid);
+      auto future = stdinPipe.writeDataAsyncAndWaitForPid(childPid);
+      // The destructor of std::future may block when the future has been
+      // created by std::async, the shared state is not ready yet, and the
+      // future represents the last reference to the shared state.
+      // In our case, the first and third criteria is always fulfilled, which
+      // means that the destructor may block (and in fact does block on at least
+      // two platforms that were tested) when the command has not finished
+      // running yet, which is exactly what we are trying to avoid when the wait
+      // flag is not set.
+      // For this reason, we append the future to a list, from which it will be
+      // removed when the run() method is called again and the shared state of
+      // the future is ready.
+      {
+        std::lock_guard<std::mutex> lock(mutex);
+        pendingFutures.push_front(std::move(future));
+      }
     }
   }
 }

executeApp/src/Command.h

@@ -1,6 +1,6 @@
 /*
- * Copyright 2018 aquenos GmbH.
- * Copyright 2018 Karlsruhe Institute of Technology.
+ * Copyright 2018-2022 aquenos GmbH.
+ * Copyright 2018-2022 Karlsruhe Institute of Technology.
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Lesser General Public License as
@@ -31,6 +31,8 @@
 #define EPICS_EXEC_COMMAND_H
 
 #include <cstdint>
+#include <forward_list>
+#include <future>
 #include <map>
 #include <mutex>
 #include <string>
@@ -183,6 +185,7 @@ class Command {
   std::map<int, std::string> arguments;
   std::map<std::string, std::string> envVars;
   mutable std::mutex mutex;
+  std::forward_list<std::future<void>> pendingFutures;
   bool running;
   std::vector<char> stderrBuffer;
   std::size_t stderrCapacity;