Merge pull request #1250 from adriendelsalle/fix-reproc-oom

Improve catching of `reproc` errors
mamba-org · Nov 3, 2021 · ca36829 · ca36829
2 parents 88c6b7c + 3c86959
commit ca36829
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 6 deletions.
diff --git a/libmamba/CMakeLists.txt b/libmamba/CMakeLists.txt
@@ -298,6 +298,7 @@ macro(libmamba_create_target target_name linkage deps_linkage output_name)
                 ${LIBSOLVEXT_BUILD_STATICRARIES}
                 ${sodium_LIBRARY_RELEASE}
                 reproc++
+                reproc
             )
 
             add_definitions("-DLIBARCHIVE_STATIC -DCURL_STATICLIB -DSOLV_BUILD_STATIC")
@@ -324,6 +325,7 @@ macro(libmamba_create_target target_name linkage deps_linkage output_name)
             ${OPENSSL_LIBRARIES}
             ${YAML_CPP_LIBRARIES}
             reproc++
+            reproc
         )
 
         target_link_libraries(${target_name} PUBLIC

diff --git a/libmamba/include/mamba/core/util.hpp b/libmamba/include/mamba/core/util.hpp
@@ -12,6 +12,8 @@
 
 #include "nlohmann/json.hpp"
 
+#include <reproc++/reproc.hpp>
+
 #include <array>
 #include <iomanip>
 #include <limits>
@@ -402,6 +404,7 @@ namespace mamba
 
     std::time_t parse_utc_timestamp(const std::string& timestamp);
 
+    void assert_reproc_success(const reproc::options& options, int status, std::error_code ec);
 }  // namespace mamba
 
 #endif  // MAMBA_UTIL_HPP
diff --git a/libmamba/src/api/install.cpp b/libmamba/src/api/install.cpp
@@ -63,12 +63,8 @@ namespace mamba
                           << " packages: " << join(", ", deps) << termcolor::reset;
         LOG_INFO << "Calling: " << join(" ", install_args);
 
-        auto [_, ec] = reproc::run(wrapped_command, options);
-
-        if (ec)
-        {
-            throw std::runtime_error(ec.message());
-        }
+        auto [status, ec] = reproc::run(wrapped_command, options);
+        assert_reproc_success(options, status, ec);
     }
 
     auto& truthy_values()

diff --git a/libmamba/src/core/util.cpp b/libmamba/src/core/util.cpp
@@ -39,6 +39,8 @@ extern "C"
 #include <thread>
 #include <condition_variable>
 
+#include <reproc/reproc.h>
+
 #include "mamba/core/context.hpp"
 #include "mamba/core/util.hpp"
 #include "mamba/core/output.hpp"
@@ -1093,4 +1095,38 @@ namespace mamba
         }
         return res;
     }
+
+    bool reproc_killed(int status)
+    {
+        return status == REPROC_SIGKILL;
+    }
+
+    bool reproc_terminated(int status)
+    {
+        return status == REPROC_SIGTERM;
+    }
+
+    void assert_reproc_success(const reproc::options& options, int status, std::error_code ec)
+    {
+        bool killed_not_an_err = (options.stop.first.action == reproc::stop::kill)
+                                 || (options.stop.second.action == reproc::stop::kill)
+                                 || (options.stop.third.action == reproc::stop::kill);
+
+        bool terminated_not_an_err = (options.stop.first.action == reproc::stop::terminate)
+                                     || (options.stop.second.action == reproc::stop::terminate)
+                                     || (options.stop.third.action == reproc::stop::terminate);
+
+        if (ec || (!killed_not_an_err && reproc_killed(status))
+            || (!terminated_not_an_err && reproc_terminated(status)))
+        {
+            if (ec)
+                LOG_ERROR << "Subprocess call failed: " << ec.message();
+            else if (reproc_killed(status))
+                LOG_ERROR << "Subprocess call failed (killed)";
+            else
+                LOG_ERROR << "Subprocess call failed (terminated)";
+            throw std::runtime_error("Subprocess call failed. Aborting.");
+        }
+    }
+
 }  // namespace mamba