Stop redirect stdout/stderr and remove close_fds=True

Stop redirect stdout/stderr and remove close_fds=True

https://docs.python.org/2.7/library/subprocess.html#popen-constructor
We can not set closed_fds True and redirect stdout/stderr at the same
time on Windows.

This CL will fix https://codereview.chromium.org/2380553002/

BUG=643139
Committed: https://chromium.googlesource.com/chromium/tools/build/+/c7d36502f22665e021c122f9c3d04ccbd568b70a

[tikuta, 2016-09-29 03:04:57]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 03:05:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tikuta, 2016-09-29 03:05:32]

tikuta@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org,
yyanagisawa@chromium.org

[commit-bot: I haz the power, 2016-09-29 03:08:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 03:08:56]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-09-29 04:51:45]

https://codereview.chromium.org/2375843005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
might be better to leave stdout, stderr and drop close_fds?
any other fds it opens?

[shinyak, 2016-09-29 05:00:16]

https://codereview.chromium.org/2375843005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (left):

https://codereview.chromium.org/2375843005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:25:
stderr=open(os.devnull, 'w'),
It looks cloudtail emits error to stderr :-(
Is it really good to close fds ???

[tikuta, 2016-09-29 05:00:29]

https://codereview.chromium.org/2375843005/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:24:
close_fds=True)
On 2016/09/29 04:51:45, ukai wrote:
> might be better to leave stdout, stderr and drop close_fds?
> any other fds it opens?

raw_io opens fds in goma module.
so, I need to close the fd in cloudtail process.

[Vadim Sh., 2016-09-29 05:06:18]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2016-09-29 05:06:19]

I think you can also use a temp file instead of stdout to pass PID from the
script to the recipe. 

e.g. teach cloudtail_utils.py to accept --pid-file parameter and use 

['start', '--cloudtail-path', self.cloudtail_path, '--pid-file',
self.m.raw_io.output()] in the recipe.

[tikuta, 2016-09-29 07:10:32]

On 2016/09/29 05:06:19, Vadim Sh. wrote:
> I think you can also use a temp file instead of stdout to pass PID from the
> script to the recipe. 
> 
> e.g. teach cloudtail_utils.py to accept --pid-file parameter and use 
> 
> ['start', '--cloudtail-path', self.cloudtail_path, '--pid-file',
> self.m.raw_io.output()] in the recipe.

Thank you for review, Vadim.
I wrote the code to use temp file.

[tikuta, 2016-09-29 07:11:24]

Description was changed from

==========
Stop redirect stdout/stderr with close_fds=True

https://docs.python.org/2.7/library/subprocess.html#popen-constructor
We can not set closed_fds True and redirect stdout/stderr at the same
time on Windows.

This CL will fix https://codereview.chromium.org/2380553002/

BUG=643139
==========

to

==========
Stop redirect stdout/stderr and remove close_fds=True

https://docs.python.org/2.7/library/subprocess.html#popen-constructor
We can not set closed_fds True and redirect stdout/stderr at the same
time on Windows.

This CL will fix https://codereview.chromium.org/2380553002/

BUG=643139
==========

[ukai, 2016-09-29 07:26:09]

lgtm

https://codereview.chromium.org/2375843005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2375843005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:143: '--pid-file',
self.m.raw_io.output(name='pid')],
use leak_to and reuse it in stop_cloudtail ?

[tikuta, 2016-09-29 07:46:15]

https://codereview.chromium.org/2375843005/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2375843005/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:143: '--pid-file',
self.m.raw_io.output(name='pid')],
On 2016/09/29 07:26:09, ukai wrote:
> use leak_to and reuse it in stop_cloudtail ?

Done.

[tikuta, 2016-09-29 07:46:22]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 07:46:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 07:49:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 07:49:55]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-09-29 07:55:02]

lgtm

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:18: self._cloudtail_pid = None
remove?

hold self.m.path['tmp_base'].join('cloudtail.pid') instead?

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:142:
leak_to=self.m.path['tmp_base'].join('pid.txt'))],
cloudtail.pid?

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:159: '--killed-pid-file',
self.m.path['tmp_base'].join('pid.txt')],
cloudtail.pid?

[tikuta, 2016-09-29 08:00:50]

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:18: self._cloudtail_pid = None
On 2016/09/29 07:55:01, ukai wrote:
> remove?
> 
> hold self.m.path['tmp_base'].join('cloudtail.pid') instead?

Done.

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:142:
leak_to=self.m.path['tmp_base'].join('pid.txt'))],
On 2016/09/29 07:55:01, ukai wrote:
> cloudtail.pid?

Done.

https://codereview.chromium.org/2375843005/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:159: '--killed-pid-file',
self.m.path['tmp_base'].join('pid.txt')],
On 2016/09/29 07:55:01, ukai wrote:
> cloudtail.pid?

Done.

[tikuta, 2016-09-29 08:00:58]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 08:01:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 08:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 08:05:00]

Dry run: This issue passed the CQ dry run.

[shinyak, 2016-09-29 09:08:49]

lgtm

[Yoshisato Yanagisawa, 2016-09-29 09:25:57]

lgtm

[Vadim Sh., 2016-09-29 18:35:15]

lgtm

I hope it works this time :)

https://codereview.chromium.org/2375843005/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:53:
os.kill(int(f.read()), signal.SIGKILL)
strictly speaking it would be better to send SIGINT and then wait for the
process to die. That way it flushes all logs correctly. It has internal 5 sec
timeout after receiving SIGINT (so it will terminate no matter what after 5
sec). This is also controllable by -flush-timeout flag.

If you don't mind loosing some logs or internal cloudtail monitoring metrics,
then killing is fine too.

In any case, this can be done in a separate CL.

[Paweł Hajdan Jr., 2016-09-29 22:26:20]

LGTM

[tikuta, 2016-09-30 03:05:56]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 03:05:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tikuta, 2016-09-30 03:07:23]

Thank you for review

https://codereview.chromium.org/2375843005/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:53:
os.kill(int(f.read()), signal.SIGKILL)
On 2016/09/29 18:35:15, Vadim Sh. wrote:
> strictly speaking it would be better to send SIGINT and then wait for the
> process to die. That way it flushes all logs correctly. It has internal 5 sec
> timeout after receiving SIGINT (so it will terminate no matter what after 5
> sec). This is also controllable by -flush-timeout flag.
> 
> If you don't mind loosing some logs or internal cloudtail monitoring metrics,
> then killing is fine too.
> 
> In any case, this can be done in a separate CL.

Done.

[commit-bot: I haz the power, 2016-09-30 03:09:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 03:09:49]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-09-30 07:59:11]

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:55: os.wait(pid,
0)
os.wait doesn't take any argument.

anyway, can we wait process which is not direct child of the process?

[tikuta, 2016-09-30 08:09:47]

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:55: os.wait(pid,
0)
On 2016/09/30 07:59:11, ukai wrote:
> os.wait doesn't take any argument.
> 
> anyway, can we wait process which is not direct child of the process?

I decided to only send SIGINT.

[tikuta, 2016-09-30 08:28:47]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 08:28:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 08:35:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 08:35:15]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-09-30 08:36:55]

lgtm

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:55: os.wait(pid,
0)
On 2016/09/30 08:09:47, tikuta wrote:
> On 2016/09/30 07:59:11, ukai wrote:
> > os.wait doesn't take any argument.
> > 
> > anyway, can we wait process which is not direct child of the process?
> 
> I decided to only send SIGINT.

comment why it uses SIGINT here?

[tikuta, 2016-09-30 11:19:51]

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:55: os.wait(pid,
0)
On 2016/09/30 08:36:54, ukai wrote:
> On 2016/09/30 08:09:47, tikuta wrote:
> > On 2016/09/30 07:59:11, ukai wrote:
> > > os.wait doesn't take any argument.
> > > 
> > > anyway, can we wait process which is not direct child of the process?
> > 
> > I decided to only send SIGINT.
> 
> comment why it uses SIGINT here?

Done.

[tikuta, 2016-09-30 11:19:58]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 11:20:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 11:23:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 11:23:53]

Dry run: This issue passed the CQ dry run.

[Vadim Sh., 2016-09-30 19:04:12]

I think sending SIGINT, but not actually waiting for termination is worse than
just sending SIGKILL, because it introduces race conditions (sort of). See my
comment inline.

https://codereview.chromium.org/2375843005/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py (right):

https://codereview.chromium.org/2375843005/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/resources/cloudtail_utils.py:55:
os.kill(int(f.read()), signal.SIGINT)
There can potentially be file locking problems on Windows if the recipe engine
exits faster than cloudtail terminates. The recipe engine may attempt a cleanup
and fail because files are still locked by cloudtail.

It appears waiting for an arbitrary PID to finish in a cross platform way is
non-trivial though...

On Linux and Mac one can use os.kill(pid, 0) and catch ESRCH OSError to know
whether PID is alive. It can be polled until it dies, see
http://stackoverflow.com/a/7654102.

On Windows, os.waitpid(pid) should work with any PID, not only child. See
https://docs.python.org/2/library/os.html#os.waitpid

[bradn, 2016-09-30 20:07:58]

The CQ bit was checked by bradnelson@google.com

[bradn, 2016-09-30 20:07:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, vadimsh@chromium.org, yyanagisawa@chromium.org,
shinyak@chromium.org, ukai@chromium.org
Link to the patchset: https://codereview.chromium.org/2375843005/#ps140001
(title: "add comment")

[commit-bot: I haz the power, 2016-09-30 20:08:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 20:12:03]

Description was changed from

==========
Stop redirect stdout/stderr and remove close_fds=True

https://docs.python.org/2.7/library/subprocess.html#popen-constructor
We can not set closed_fds True and redirect stdout/stderr at the same
time on Windows.

This CL will fix https://codereview.chromium.org/2380553002/

BUG=643139
==========

to

==========
Stop redirect stdout/stderr and remove close_fds=True

https://docs.python.org/2.7/library/subprocess.html#popen-constructor
We can not set closed_fds True and redirect stdout/stderr at the same
time on Windows.

This CL will fix https://codereview.chromium.org/2380553002/

BUG=643139

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/c7d36502f22665e021c1...
==========

[commit-bot: I haz the power, 2016-09-30 20:12:04]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/tools/build/+/c7d36502f22665e021c1...

[Vadim Sh., 2016-09-30 21:04:43]

On 2016/09/30 20:12:04, commit-bot: I haz the power wrote:
> Committed patchset #8 (id:140001) as
>
https://chromium.googlesource.com/chromium/tools/build/+/c7d36502f22665e021c1...

(We landed this because client.wasm.llvm builders were broken).