From 618194207fc7d093cc9a353561700ab8f8684735 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 19 Dec 2022 18:00:22 +0300 Subject: Fix simple and script tests to correctly terminate processes which don't close stderr on exit --- libbuild2/test/rule.cxx | 98 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 84 insertions(+), 14 deletions(-) (limited to 'libbuild2/test') diff --git a/libbuild2/test/rule.cxx b/libbuild2/test/rule.cxx index 0ee7641..2d02d39 100644 --- a/libbuild2/test/rule.cxx +++ b/libbuild2/test/rule.cxx @@ -668,6 +668,16 @@ namespace build2 // bool terminated = false; + // True if this process has been terminated but we failed to read out + // its stderr stream in the reasonable timeframe (2 seconds) after the + // termination. + // + // Note that this may happen if there is a still running child process + // of the terminated process which has inherited the parent's stderr + // file descriptor. + // + bool unread_stderr = false; + pipe_process* prev; // NULL for the left-most program. pipe_process* next; // Left-most program for the right-most program. @@ -780,10 +790,14 @@ namespace build2 // Read out all the pipeline's buffered strerr streams watching for // the deadline, if specified. If the deadline is reached, then - // terminate the whole pipeline, reset the deadline to nullopt, and - // continue reading. Note that the further reading will be performed - // without timeout. This, however, is fine since all the processes are - // terminated and we only need to read out the buffered data. + // terminate the whole pipeline, move the deadline by another 2 + // seconds, and continue reading. + // + // Note that we assume that this timeout increment is normally + // sufficient to read out the buffered data written by the already + // terminated processes. If, however, that's not the case (see + // pipe_process for the possible reasons), then we just set + // unread_stderr flag to true for such processes and bail out. // // Also note that this implementation is inspired by the // script::run_pipe::read_pipe() lambda. @@ -800,6 +814,7 @@ namespace build2 } optional dl (deadline); + bool terminated (false); for (size_t unread (fds.size ()); unread != 0;) { @@ -814,9 +829,38 @@ namespace build2 if (*dl <= now || ifdselect (fds, *dl - now) == 0) { - term_pipe (&pp); - dl = nullopt; - continue; + if (!terminated) + { + term_pipe (&pp); + terminated = true; + + dl = system_clock::now () + chrono::seconds (2); + continue; + } + else + { + for (fdselect_state& s: fds) + { + if (s.fd != nullfd) + { + pipe_process* p (static_cast (s.data)); + + p->unread_stderr = true; + + // Let's also close the stderr stream not to confuse + // diag_buffer::close() (see script::read() for + // details). + // + try + { + p->dbuf.is.close (); + } + catch (const io_error&) {} + } + } + + break; + } } } else @@ -869,9 +913,9 @@ namespace build2 // Iterate over the pipeline processes left to right, printing their // stderr if buffered and issuing the diagnostics if the exit code is - // not available (terminated abnormally or due to a deadline) or is - // non-zero. Afterwards, fail if any of the processes didn't terminate - // normally with zero code. + // not available (terminated abnormally or due to a deadline), is + // non-zero, or stderr was not fully read. Afterwards, fail if any of + // such a faulty processes were encountered. // // Note that we only issue diagnostics for the first failure. // @@ -920,18 +964,44 @@ namespace build2 { diag_record dr; - if (!pe || !pe->normal () || pe->code () != 0) + // Note that there can be a race, so that the process we have + // terminated due to reaching the deadline has in fact exited + // normally. Thus, the 'unread stderr' situation can also happen + // to a successfully terminated process. If that's the case, we + // report this problem as the main error and the secondary error + // otherwise. + // + if (!pe || + !pe->normal () || + pe->code () != 0 || + p->unread_stderr) { fail = true; dr << error << "test " << t << " failed" // Multi test: test 1. << error << "process " << p->args[0] << ' '; - if (pe) - dr << *pe; - else + if (!pe) + { dr << "terminated: execution timeout expired"; + if (p->unread_stderr) + dr << error << "stderr not closed after exit"; + } + else if (!pe->normal () || pe->code () != 0) + { + dr << *pe; + + if (p->unread_stderr) + dr << error << "stderr not closed after exit"; + } + else + { + assert (p->unread_stderr); + + dr << "stderr not closed after exit"; + } + if (verb == 1) { dr << info << "test command line: "; -- cgit v1.1