Sitelet https://github.com/linuxdeploy/linuxdeploy/commit/11ca1efddb43162e552df994586caef308bd037e
Skip to content

Commit 11ca1ef

Browse files
TheAssassinTheAssassin
committed
Make sure to read until EOF from subprocesses
1 parent 0e89061 commit 11ca1ef

3 files changed

Lines changed: 61 additions & 30 deletions

File tree

‎src/plugin/plugin_process_handler.cpp‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ namespace linuxdeploy {
4343
std::string stream_name_;
4444
ldLog log_;
4545
bool print_prefix_in_next_iteration_;
46+
bool eof = false;
4647

4748
pipe_to_be_logged(int pipe_fd, std::string stream_name) : reader_(pipe_fd),
4849
stream_name_(std::move(stream_name)),
@@ -62,12 +63,17 @@ namespace linuxdeploy {
6263
// since we have our own ldLog instance for every pipe, we can get away with this rather small read buffer
6364
subprocess::subprocess_result_buffer_t intermediate_buffer(4096);
6465

66+
if (pipe_to_be_logged.eof) {
67+
break;
68+
}
69+
6570
// (try to) read from pipe
6671
const auto bytes_read = pipe_to_be_logged.reader_.read(intermediate_buffer);
6772

68-
// no action required in case we have not read anything from the pipe
69-
if (bytes_read <= 0) {
70-
continue;
73+
// 0 means EOF
74+
if (bytes_read == 0) {
75+
pipe_to_be_logged.eof = true;
76+
break;
7177
}
7278

7379
// we just trim the buffer to the bytes we read (makes the code below easier)
@@ -119,7 +125,12 @@ namespace linuxdeploy {
119125
if (proc.is_running()) {
120126
// reduce load on CPU
121127
std::this_thread::sleep_for(std::chrono::milliseconds(50));
122-
} else {
128+
}
129+
130+
// once all buffers are EOF, we can stop reading
131+
if (std::all_of(pipes_to_be_logged.begin(), pipes_to_be_logged.end(), [](const pipe_to_be_logged& pipe_state) {
132+
return pipe_state.eof;
133+
})) {
123134
break;
124135
}
125136
}

‎src/subprocess/pipe_reader.cpp‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,21 @@ pipe_reader::pipe_reader(int pipe_fd) : pipe_fd_(pipe_fd) {
1717
}
1818

1919
size_t pipe_reader::read(std::vector<std::string::value_type>& buffer) const {
20-
ssize_t rv = ::read(pipe_fd_, buffer.data(), buffer.size());
20+
for (;;) {
21+
ssize_t rv = ::read(pipe_fd_, buffer.data(), buffer.size());
2122

22-
if (rv == -1) {
23-
// no data available
24-
if (errno == EAGAIN)
25-
return 0;
23+
if (rv == -1) {
24+
switch (errno) {
25+
// retry in case data is currently not available
26+
case EINTR:
27+
case EAGAIN:
28+
continue;
29+
default:
30+
// TODO: introduce custom subprocess_error
31+
throw std::runtime_error{"unexpected error reading from pipe: " + std::string(strerror(errno))};
32+
}
33+
}
2634

27-
// TODO: introduce custom subprocess_error
28-
throw std::runtime_error{"unexpected error reading from pipe: " + std::string(strerror(errno))};
35+
return rv;
2936
}
30-
31-
return rv;
3237
}

‎src/subprocess/subprocess.cpp‎

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,55 +27,70 @@ namespace linuxdeploy {
2727
subprocess_result subprocess::run() const {
2828
process proc{args_, env_};
2929

30+
class PipeState {
31+
public:
32+
pipe_reader reader;
33+
subprocess_result_buffer_t buffer;
34+
bool eof = false;
35+
36+
explicit PipeState(int fd) : reader(fd) {}
37+
};
38+
3039
// create pipe readers and empty buffers for both stdout and stderr
3140
// we manage them in this (admittedly, kind of complex-looking) array so we can later easily perform the
3241
// operations in a loop
33-
std::array<std::pair<pipe_reader, subprocess_result_buffer_t>, 2> buffers{
34-
std::make_pair(pipe_reader(proc.stdout_fd()), subprocess_result_buffer_t{}),
35-
std::make_pair(pipe_reader(proc.stderr_fd()), subprocess_result_buffer_t{}),
42+
std::array<PipeState, 2> buffers = {
43+
PipeState(proc.stdout_fd()),
44+
PipeState(proc.stderr_fd())
3645
};
3746

3847
for (;;) {
39-
for (auto& pair : buffers) {
40-
// make code more readable
41-
auto& reader = pair.first;
42-
auto& buffer = pair.second;
43-
48+
for (auto& pipe_state : buffers) {
4449
// read some bytes into smaller intermediate buffer to prevent either of the pipes to overflow
4550
// the results are immediately appended to the main buffer
4651
subprocess_result_buffer_t intermediate_buffer(4096);
4752

4853
// (try to) read all available data from pipe
4954
for (;;) {
50-
const auto bytes_read = reader.read(intermediate_buffer);
55+
if (pipe_state.eof) {
56+
break;
57+
}
58+
59+
const auto bytes_read = pipe_state.reader.read(intermediate_buffer);
5160

61+
// 0 means EOF
5262
if (bytes_read == 0) {
63+
pipe_state.eof = true;
5364
break;
5465
}
5566

5667
// append to main buffer
57-
buffer.reserve(buffer.size() + bytes_read);
68+
pipe_state.buffer.reserve(pipe_state.buffer.size() + bytes_read);
5869
std::copy(intermediate_buffer.begin(), (intermediate_buffer.begin() + bytes_read),
59-
std::back_inserter(buffer));
70+
std::back_inserter(pipe_state.buffer));
6071
}
6172
}
6273

63-
// do-while might be a little more elegant, but we can save this one unnecessary sleep, so...
6474
if (proc.is_running()) {
65-
// reduce load on CPU
75+
// reduce load on CPU until EOF
6676
std::this_thread::sleep_for(std::chrono::milliseconds(50));
67-
} else {
77+
}
78+
79+
// once all buffers are EOF, we can stop reading
80+
if (std::all_of(buffers.begin(), buffers.end(), [](const PipeState& pipe_state) {
81+
return pipe_state.eof;
82+
})) {
6883
break;
6984
}
7085
}
7186

7287
// make sure contents are null-terminated
73-
buffers[0].second.emplace_back('\0');
74-
buffers[1].second.emplace_back('\0');
88+
buffers[0].buffer.emplace_back('\0');
89+
buffers[1].buffer.emplace_back('\0');
7590

7691
auto exit_code = proc.close();
7792

78-
return subprocess_result{exit_code, buffers[0].second, buffers[1].second};
93+
return subprocess_result{exit_code, buffers[0].buffer, buffers[1].buffer};
7994
}
8095

8196
std::string subprocess::check_output() const {

0 commit comments

Comments
 (0)