New issue
Advanced search Search tips

Issue 733536 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

autotest: no_pipes for BgJob does not work.

Project Member Reported by hidehiko@chromium.org, Jun 15 2017

Issue description

The flag is introduced to address  crbug.com/279312 .

According to the doc for the arg, it tried to proxy the stdout/stderr from the subprocess to LoggingFile.
However, it does not work.
If no_pipes is set, LoggingFile instance is directly passed to the subprocess.Popen(). The ctor accepts the file-like object which supports fileno. LoggingFile has fileno(), but it is a kind of fake (pipe(), which is not read by anyone) and subprocess outputs the data to the pipe.

As a result, the data is just discarded from the users' perspective. Also, if too many data is output, pipe would be full so the subprocess would be stuck.

Considering that;
1) Currently nobody complaining that the output is simply discarded.
2) Currently it is only for limited usage (master-ssh connection process).
3) No easy/simple approach to redirect the data correctly to the LoggingFile concurrently.

it looks ok to give up proxying, and instead just to redirect subprocess's stdout/stderr to /dev/null.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/d939d66604032ed727ce99cc0455e2e04f0728cb

commit d939d66604032ed727ce99cc0455e2e04f0728cb
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Thu Jun 15 22:17:15 2017

autotest: Fix no_pipes behavior of BgJob.

Currently, if no_pipes is set, logging_manager.LoggingFile instance
is directly passed to subprocess.Popen. However, this does not work,
because subprocess.Popen relies on fileno(), but LoggingFile
provides just a fake FD.
To make matters worse, because it is a pipe, if a subprocess output
too many data, it would be blocked because no one reads it.

Considering current usage (master ssh connection process),
this CL just drops the message proxying code, and, instead, supports
DEVNULL, meaning to discard all the data from subprocess,
and |unjoinable|, meaning the BgJob running on background.
From coding point of view, |unjoinable| option is just for sanity check
which no_pipes option did.

Then, set DEVNULL to BgJob() with no_pipes=True cases.
From users perspective, this should be almost no-op,
because, even now the stdout/stderr data is just discarded.

Along with the change, output_prepare() looks not useful,
but to make the DEVNULL support complicated, so dropped it.

BUG= chromium:733536 
TEST=Ran test_that locally. Ran bot.

Change-Id: I7aaadbc16b1ff54f222cc658a3d127fef246287d
Reviewed-on: https://chromium-review.googlesource.com/532468
Commit-Ready: Hidehiko Abe <hidehiko@chromium.org>
Tested-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[modify] https://crrev.com/d939d66604032ed727ce99cc0455e2e04f0728cb/server/hosts/abstract_ssh.py
[modify] https://crrev.com/d939d66604032ed727ce99cc0455e2e04f0728cb/client/common_lib/utils.py

Status: Fixed (was: Started)

Comment 3 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment