diff --git a/src/plugin/plugin_process_handler.cpp b/src/plugin/plugin_process_handler.cpp index 1a4a4dc..f0d5dbc 100644 --- a/src/plugin/plugin_process_handler.cpp +++ b/src/plugin/plugin_process_handler.cpp @@ -43,6 +43,7 @@ namespace linuxdeploy { std::string stream_name_; ldLog log_; bool print_prefix_in_next_iteration_; + bool eof = false; pipe_to_be_logged(int pipe_fd, std::string stream_name) : reader_(pipe_fd), stream_name_(std::move(stream_name)), @@ -62,12 +63,17 @@ namespace linuxdeploy { // since we have our own ldLog instance for every pipe, we can get away with this rather small read buffer subprocess::subprocess_result_buffer_t intermediate_buffer(4096); + if (pipe_to_be_logged.eof) { + break; + } + // (try to) read from pipe const auto bytes_read = pipe_to_be_logged.reader_.read(intermediate_buffer); - // no action required in case we have not read anything from the pipe - if (bytes_read <= 0) { - continue; + // 0 means EOF + if (bytes_read == 0) { + pipe_to_be_logged.eof = true; + break; } // we just trim the buffer to the bytes we read (makes the code below easier) @@ -119,7 +125,12 @@ namespace linuxdeploy { if (proc.is_running()) { // reduce load on CPU std::this_thread::sleep_for(std::chrono::milliseconds(50)); - } else { + } + + // once all buffers are EOF, we can stop reading + if (std::all_of(pipes_to_be_logged.begin(), pipes_to_be_logged.end(), [](const pipe_to_be_logged& pipe_state) { + return pipe_state.eof; + })) { break; } } diff --git a/src/subprocess/pipe_reader.cpp b/src/subprocess/pipe_reader.cpp index 1ce2a54..fef8384 100644 --- a/src/subprocess/pipe_reader.cpp +++ b/src/subprocess/pipe_reader.cpp @@ -17,16 +17,21 @@ pipe_reader::pipe_reader(int pipe_fd) : pipe_fd_(pipe_fd) { } size_t pipe_reader::read(std::vector& buffer) const { - ssize_t rv = ::read(pipe_fd_, buffer.data(), buffer.size()); + for (;;) { + ssize_t rv = ::read(pipe_fd_, buffer.data(), buffer.size()); - if (rv == -1) { - // no data available - if (errno == EAGAIN) - return 0; + if (rv == -1) { + switch (errno) { + // retry in case data is currently not available + case EINTR: + case EAGAIN: + continue; + default: + // TODO: introduce custom subprocess_error + throw std::runtime_error{"unexpected error reading from pipe: " + std::string(strerror(errno))}; + } + } - // TODO: introduce custom subprocess_error - throw std::runtime_error{"unexpected error reading from pipe: " + std::string(strerror(errno))}; + return rv; } - - return rv; } diff --git a/src/subprocess/subprocess.cpp b/src/subprocess/subprocess.cpp index 566b67a..4a7479a 100644 --- a/src/subprocess/subprocess.cpp +++ b/src/subprocess/subprocess.cpp @@ -27,55 +27,70 @@ namespace linuxdeploy { subprocess_result subprocess::run() const { process proc{args_, env_}; + class PipeState { + public: + pipe_reader reader; + subprocess_result_buffer_t buffer; + bool eof = false; + + explicit PipeState(int fd) : reader(fd) {} + }; + // create pipe readers and empty buffers for both stdout and stderr // we manage them in this (admittedly, kind of complex-looking) array so we can later easily perform the // operations in a loop - std::array, 2> buffers{ - std::make_pair(pipe_reader(proc.stdout_fd()), subprocess_result_buffer_t{}), - std::make_pair(pipe_reader(proc.stderr_fd()), subprocess_result_buffer_t{}), + std::array buffers = { + PipeState(proc.stdout_fd()), + PipeState(proc.stderr_fd()) }; for (;;) { - for (auto& pair : buffers) { - // make code more readable - auto& reader = pair.first; - auto& buffer = pair.second; - + for (auto& pipe_state : buffers) { // read some bytes into smaller intermediate buffer to prevent either of the pipes to overflow // the results are immediately appended to the main buffer subprocess_result_buffer_t intermediate_buffer(4096); // (try to) read all available data from pipe for (;;) { - const auto bytes_read = reader.read(intermediate_buffer); + if (pipe_state.eof) { + break; + } + const auto bytes_read = pipe_state.reader.read(intermediate_buffer); + + // 0 means EOF if (bytes_read == 0) { + pipe_state.eof = true; break; } // append to main buffer - buffer.reserve(buffer.size() + bytes_read); + pipe_state.buffer.reserve(pipe_state.buffer.size() + bytes_read); std::copy(intermediate_buffer.begin(), (intermediate_buffer.begin() + bytes_read), - std::back_inserter(buffer)); + std::back_inserter(pipe_state.buffer)); } } - // do-while might be a little more elegant, but we can save this one unnecessary sleep, so... if (proc.is_running()) { - // reduce load on CPU + // reduce load on CPU until EOF std::this_thread::sleep_for(std::chrono::milliseconds(50)); - } else { + } + + // once all buffers are EOF, we can stop reading + if (std::all_of(buffers.begin(), buffers.end(), [](const PipeState& pipe_state) { + return pipe_state.eof; + })) { break; } } // make sure contents are null-terminated - buffers[0].second.emplace_back('\0'); - buffers[1].second.emplace_back('\0'); + buffers[0].buffer.emplace_back('\0'); + buffers[1].buffer.emplace_back('\0'); auto exit_code = proc.close(); - return subprocess_result{exit_code, buffers[0].second, buffers[1].second}; + return subprocess_result{exit_code, buffers[0].buffer, buffers[1].buffer}; } std::string subprocess::check_output() const {