Skip to content

bpo-45410: libregrtest -jN writes stderr into stdout #28819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 18 additions & 22 deletions Lib/test/libregrtest/runtest_mp.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ def run_test_in_subprocess(testname: str, ns: Namespace) -> subprocess.Popen:
kw['start_new_session'] = True
return subprocess.Popen(cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
# bpo-45410: Write stderr into stdout to keep
# messages order
stderr=subprocess.STDOUT,
universal_newlines=True,
close_fds=(os.name != 'nt'),
cwd=os_helper.SAVEDCWD,
Expand Down Expand Up @@ -114,8 +116,8 @@ def stop(self):

class MultiprocessResult(NamedTuple):
result: TestResult
# bpo-45410: stderr is written into stdout to keep messages order
stdout: str
stderr: str
error_msg: str


Expand Down Expand Up @@ -195,11 +197,10 @@ def mp_result_error(
self,
test_result: TestResult,
stdout: str = '',
stderr: str = '',
err_msg=None
) -> MultiprocessResult:
test_result.duration_sec = time.monotonic() - self.start_time
return MultiprocessResult(test_result, stdout, stderr, err_msg)
return MultiprocessResult(test_result, stdout, err_msg)

def _run_process(self, test_name: str) -> tuple[int, str, str]:
self.start_time = time.monotonic()
Expand All @@ -223,13 +224,14 @@ def _run_process(self, test_name: str) -> tuple[int, str, str]:
raise ExitThread

try:
stdout, stderr = popen.communicate(timeout=self.timeout)
# bpo-45410: stderr is written into stdout
stdout, _ = popen.communicate(timeout=self.timeout)
retcode = popen.returncode
assert retcode is not None
except subprocess.TimeoutExpired:
if self._stopped:
# kill() has been called: communicate() fails
# on reading closed stdout/stderr
# kill() has been called: communicate() fails on reading
# closed stdout
raise ExitThread

# On timeout, kill the process
Expand All @@ -238,20 +240,19 @@ def _run_process(self, test_name: str) -> tuple[int, str, str]:
# None means TIMEOUT for the caller
retcode = None
# bpo-38207: Don't attempt to call communicate() again: on it
# can hang until all child processes using stdout and stderr
# can hang until all child processes using stdout
# pipes completes.
stdout = stderr = ''
stdout = ''
except OSError:
if self._stopped:
# kill() has been called: communicate() fails
# on reading closed stdout/stderr
# on reading closed stdout
raise ExitThread
raise
else:
stdout = stdout.strip()
stderr = stderr.rstrip()

return (retcode, stdout, stderr)
return (retcode, stdout)
except:
self._kill()
raise
Expand All @@ -261,10 +262,10 @@ def _run_process(self, test_name: str) -> tuple[int, str, str]:
self.current_test_name = None

def _runtest(self, test_name: str) -> MultiprocessResult:
retcode, stdout, stderr = self._run_process(test_name)
retcode, stdout = self._run_process(test_name)

if retcode is None:
return self.mp_result_error(Timeout(test_name), stdout, stderr)
return self.mp_result_error(Timeout(test_name), stdout)

err_msg = None
if retcode != 0:
Expand All @@ -282,10 +283,9 @@ def _runtest(self, test_name: str) -> MultiprocessResult:
err_msg = "Failed to parse worker JSON: %s" % exc

if err_msg is not None:
return self.mp_result_error(ChildError(test_name),
stdout, stderr, err_msg)
return self.mp_result_error(ChildError(test_name), stdout, err_msg)

return MultiprocessResult(result, stdout, stderr, err_msg)
return MultiprocessResult(result, stdout, err_msg)

def run(self) -> None:
while not self._stopped:
Expand All @@ -309,10 +309,8 @@ def run(self) -> None:
def _wait_completed(self) -> None:
popen = self._popen

# stdout and stderr must be closed to ensure that communicate()
# does not hang
# stdout must be closed to ensure that communicate() does not hang
popen.stdout.close()
popen.stderr.close()

try:
popen.wait(JOIN_TIMEOUT)
Expand Down Expand Up @@ -449,8 +447,6 @@ def _process_result(self, item: QueueOutput) -> bool:

if mp_result.stdout:
print(mp_result.stdout, flush=True)
if mp_result.stderr and not self.ns.pgo:
print(mp_result.stderr, file=sys.stderr, flush=True)

if must_stop(mp_result.result, self.ns):
return True
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
When libregrtest spawns a worker process, stderr is now written into stdout
to keep messages order. Use a single pipe for stdout and stderr, rather than
two pipes. Previously, messages were out of order which made analysis of
buildbot logs harder Patch by Victor Stinner.