From 52eee95bf8665210af5dd6706042d4730c6dba7d Mon Sep 17 00:00:00 2001 From: patrick96 Date: Wed, 25 Nov 2020 12:16:06 +0100 Subject: [PATCH] controller: Detach shell commands from polybar Shell commands triggered from action tags used to block polybar until they finished. Since we are not actually interested in the output of the commands, it makes sense to run them completely detached from polybar and have polybar not block when executing these commands. Now the spawned child processes no longer get killed when polybar exits. This is fine because polybar is not responsible for these processes since they were explicitly started by the user through click commands. Ref: #770 Ref: #1680 --- include/components/controller.hpp | 2 -- include/utils/process.hpp | 9 ++++--- src/components/controller.cpp | 8 ++---- src/utils/command.cpp | 27 +++++++------------ src/utils/process.cpp | 43 +++++++++++++++++++++++------- tests/CMakeLists.txt | 1 + tests/unit_tests/utils/process.cpp | 34 +++++++++++++++++++++++ 7 files changed, 86 insertions(+), 38 deletions(-) create mode 100644 tests/unit_tests/utils/process.cpp diff --git a/include/components/controller.hpp b/include/components/controller.hpp index 18c3e679..f70156bb 100644 --- a/include/components/controller.hpp +++ b/include/components/controller.hpp @@ -20,8 +20,6 @@ POLYBAR_NS enum class alignment; class bar; -template -class command; class config; class connection; class inotify_watch; diff --git a/include/utils/process.hpp b/include/utils/process.hpp index c5d91c01..eb380532 100644 --- a/include/utils/process.hpp +++ b/include/utils/process.hpp @@ -8,19 +8,20 @@ namespace process_util { bool in_parent_process(pid_t pid); bool in_forked_process(pid_t pid); - void redirect_process_output_to_dev_null(); + void redirect_stdio_to_dev_null(); + + pid_t fork_detached(std::function const& lambda); void exec(char* cmd, char** args); void exec_sh(const char* cmd); - pid_t wait_for_completion(pid_t process_id, int* status_addr, int waitflags = 0); + pid_t wait_for_completion(pid_t process_id, int* status_addr = nullptr, int waitflags = 0); pid_t wait_for_completion(int* status_addr, int waitflags = 0); - pid_t wait_for_completion(pid_t process_id); pid_t wait_for_completion_nohang(pid_t process_id, int* status); pid_t wait_for_completion_nohang(int* status); pid_t wait_for_completion_nohang(); bool notify_childprocess(); -} +} // namespace process_util POLYBAR_NS_END diff --git a/src/components/controller.cpp b/src/components/controller.cpp index d809a1ca..f55a05d1 100644 --- a/src/components/controller.cpp +++ b/src/components/controller.cpp @@ -15,9 +15,9 @@ #include "modules/meta/event_handler.hpp" #include "modules/meta/factory.hpp" #include "utils/actions.hpp" -#include "utils/command.hpp" #include "utils/factory.hpp" #include "utils/inotify.hpp" +#include "utils/process.hpp" #include "utils/string.hpp" #include "utils/time.hpp" #include "x11/connection.hpp" @@ -561,12 +561,8 @@ void controller::process_inputdata() { try { // Run input as command if it's not an input for a module m_log.info("Forwarding command to shell... (input: %s)", cmd); - m_log.info("Executing shell command: %s", cmd); - - auto shell_cmd = command_util::make_command(move(cmd)); - shell_cmd->exec(); - shell_cmd.reset(); + process_util::fork_detached([cmd] { process_util::exec_sh(cmd.c_str()); }); process_update(true); } catch (const application_error& err) { m_log.err("controller: Error while forwarding input to shell -> %s", err.what()); diff --git a/src/utils/command.cpp b/src/utils/command.cpp index ef44c0c5..2f2afc2c 100644 --- a/src/utils/command.cpp +++ b/src/utils/command.cpp @@ -1,12 +1,14 @@ +#include "utils/command.hpp" + #include #include + #include #include #include #include #include "errors.hpp" -#include "utils/command.hpp" #include "utils/io.hpp" #include "utils/process.hpp" @@ -31,20 +33,11 @@ command::~command() { * Execute the command */ int command::exec(bool wait_for_completion) { - if ((m_forkpid = fork()) == -1) { - throw system_error("Failed to fork process"); - } - - if (process_util::in_forked_process(m_forkpid)) { - setpgid(m_forkpid, 0); - process_util::redirect_process_output_to_dev_null(); - process_util::exec_sh(m_cmd.c_str()); - } else { - if (wait_for_completion) { - auto status = wait(); - m_forkpid = -1; - return status; - } + m_forkpid = process_util::fork_detached([m_cmd = m_cmd] { process_util::exec_sh(m_cmd.c_str()); }); + if (wait_for_completion) { + auto status = wait(); + m_forkpid = -1; + return status; } return EXIT_SUCCESS; @@ -90,7 +83,7 @@ int command::wait() { } } while (!WIFEXITED(m_forkstatus) && !WIFSIGNALED(m_forkstatus)); - return m_forkstatus; + return WEXITSTATUS(m_forkstatus); } /** @@ -104,7 +97,7 @@ pid_t command::get_pid() { * Get command exit status */ int command::get_exit_status() { - return m_forkstatus; + return WEXITSTATUS(m_forkstatus); } command::command(const polybar::logger& logger, std::string cmd) diff --git a/src/utils/process.cpp b/src/utils/process.cpp index 79b1bce0..f5ec0857 100644 --- a/src/utils/process.cpp +++ b/src/utils/process.cpp @@ -1,6 +1,8 @@ #include "utils/process.hpp" #include +#include +#include #include #include @@ -25,7 +27,10 @@ namespace process_util { return pid == 0; } - void redirect_process_output_to_dev_null() { + /** + * Redirects all io fds (stdin, stdout, stderr) of the current process to /dev/null. + */ + void redirect_stdio_to_dev_null() { auto redirect = [](int fd_to_redirect) { int fd = open("/dev/null", O_WRONLY); if (fd < 0 || dup2(fd, fd_to_redirect) < 0) { @@ -34,10 +39,38 @@ namespace process_util { close(fd); }; + redirect(STDIN_FILENO); redirect(STDOUT_FILENO); redirect(STDERR_FILENO); } + /** + * Forks a child process and completely detaches it. + * + * In the child process, the given lambda function is executed. + * + * Use this if you want to run a command and just forget about it. + * + * \returns The PID of the child process + */ + pid_t fork_detached(std::function const& lambda) { + pid_t pid = fork(); + switch (pid) { + case -1: + throw runtime_error("fork_detached: Unable to fork: " + string(strerror(errno))); + case 0: + // Child + setsid(); + umask(0); + redirect_stdio_to_dev_null(); + lambda(); + _Exit(0); + break; + default: + return pid; + } + } + /** * Execute command */ @@ -76,14 +109,6 @@ namespace process_util { return wait_for_completion(-1, status_addr, waitflags); } - /** - * Wait for child process - */ - pid_t wait_for_completion(pid_t process_id) { - int status = 0; - return wait_for_completion(process_id, &status); - } - /** * Non-blocking wait * diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2186c71d..c90504b8 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -55,6 +55,7 @@ add_unit_test(utils/memory unit_tests) add_unit_test(utils/scope unit_tests) add_unit_test(utils/string unit_tests) add_unit_test(utils/file) +add_unit_test(utils/process) add_unit_test(components/command_line) add_unit_test(components/bar) add_unit_test(components/parser) diff --git a/tests/unit_tests/utils/process.cpp b/tests/unit_tests/utils/process.cpp new file mode 100644 index 00000000..cfd8351c --- /dev/null +++ b/tests/unit_tests/utils/process.cpp @@ -0,0 +1,34 @@ +#include "utils/process.hpp" + +#include +#include +#include + +#include +#include + +#include "common/test.hpp" + +using namespace polybar; +using namespace process_util; + +TEST(ForkDetached, is_detached) { + pid_t pid = fork_detached([] { exec_sh("sleep 0.1"); }); + int status; + + pid_t res = process_util::wait_for_completion_nohang(pid, &status); + + ASSERT_NE(res, -1); + + EXPECT_FALSE(WIFEXITED(status)); +} + +TEST(ForkDetached, exit_code) { + pid_t pid = fork_detached([] { exec_sh("exit 42"); }); + int status = 0; + pid_t res = waitpid(pid, &status, 0); + + EXPECT_EQ(res, pid); + + EXPECT_EQ(WEXITSTATUS(status), 42); +}