Kill subprocesses on KeyboardInterrupt.

Kill subprocesses on KeyboardInterrupt.

SVN traps SIGINT and attempts to clean itself up, but this results in hangs
waiting for TCP. This patch does two things: daemonizes worker threads so they
are culled when the main thread dies (is ctrl-C'd) and keeps track of spawned
subprocesses to kill any remaining ones when the main program is ctrl-C'd.

A user ctrl-C'ing gclient has to manually terminate hung SVN processes, so this
introduces no extra data loss or hazard. stracing a hung SVN process shows that
it is indeed hanging on TCP reads after receiving a SIGINT, implying there is an
underlying but in the SVN binary.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=198205

[Mike Stip (use stip instead), 2013-05-03 15:40:45]

ptal

[M-A Ruel, 2013-05-03 16:39:01]

https://codereview.chromium.org/14759006/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/14759006/diff/1/gclient.py#newcode1789
gclient.py:1789: except KeyboardInterrupt:
Why not inside Main()?

https://codereview.chromium.org/14759006/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/14759006/diff/1/gclient_utils.py#newcode333
gclient_utils.py:333: cls._instance = object.__new__(cls, *args, **kwargs)
I don't think it's a good idea. Just create a global variable, it's cleaner and
way more logical.

https://codereview.chromium.org/14759006/diff/1/gclient_utils.py#newcode723
gclient_utils.py:723: raise
Then the lines 731-735 should be in a finally

[Mike Stip (use stip instead), 2013-05-03 17:30:39]

Addressed the comments. ptal!

[M-A Ruel, 2013-05-03 20:54:12]

https://chromiumcodereview.appspot.com/14759006/diff/10001/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/14759006/diff/10001/gclient_utils.py#n...
gclient_utils.py:326: GCLIENT_CHILDREN = {}
Why a dict instead of a list?

https://chromiumcodereview.appspot.com/14759006/diff/10001/gclient_utils.py#n...
gclient_utils.py:346: for child in GCLIENT_CHILDREN.values():
with GCLIENT_CHILDREN_LOCK:
  zombies = [c for c in GCLIENT_CHILDREN.itervalues() if c.poll() is None]

for zombie in zombies:
  try:
    child.kill()
  except OSError:
    pass

with GCLIENT_CHILDREN_LOCK:
  for k in GCLIENT_CHILDREN.keys():
    if GCLIENT_CHILDREN[k].poll() is not None:
      GCLIENT_CHILDREN.pop(k)


This way you don't kill with lock.

[Dirk Pranke, 2013-05-03 21:04:13]

It will be great to fix this, but something tells me that kill() isn't reliable
on windows ... be sure to test it there.

[Mike Stip (use stip instead), 2013-05-03 21:14:16]

On 2013/05/03 21:04:13, Dirk Pranke wrote:
> It will be great to fix this, but something tells me that kill() isn't
reliable
> on windows ... be sure to test it there.

Will test, but I'd like to point out that this uses subprocess2's kill(), which
adds in hooks for windows:
http://src.chromium.org/svn/trunk/tools/depot_tools/subprocess2.py

[Mike Stip (use stip instead), 2013-05-03 21:16:08]

Addressed comments. ptal!

https://chromiumcodereview.appspot.com/14759006/diff/10001/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/14759006/diff/10001/gclient_utils.py#n...
gclient_utils.py:326: GCLIENT_CHILDREN = {}
On 2013/05/03 20:54:12, Marc-Antoine Ruel wrote:
> Why a dict instead of a list?

Since I figured it was better to delete via pid than by object, but I guess
objects work OK too. Changed.

https://chromiumcodereview.appspot.com/14759006/diff/10001/gclient_utils.py#n...
gclient_utils.py:346: for child in GCLIENT_CHILDREN.values():
On 2013/05/03 20:54:12, Marc-Antoine Ruel wrote:
> with GCLIENT_CHILDREN_LOCK:
>   zombies = [c for c in GCLIENT_CHILDREN.itervalues() if c.poll() is None]
> 
> for zombie in zombies:
>   try:
>     child.kill()
>   except OSError:
>     pass
> 
> with GCLIENT_CHILDREN_LOCK:
>   for k in GCLIENT_CHILDREN.keys():
>     if GCLIENT_CHILDREN[k].poll() is not None:
>       GCLIENT_CHILDREN.pop(k)
> 
> 
> This way you don't kill with lock.

Done.

[M-A Ruel, 2013-05-03 21:26:42]

lgtm, I agree list.remove() could become a bottleneck, so I'm fine with a dict.

https://chromiumcodereview.appspot.com/14759006/diff/19001/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/14759006/diff/19001/gclient_utils.py#n...
gclient_utils.py:359: if GCLIENT_CHILDREN:
return bool(GCLIENT_CHILDREN)

[commit-bot: I haz the power, 2013-05-03 21:55:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xusydoc@chromium.org/14759006/24001

[commit-bot: I haz the power, 2013-05-03 21:57:56]

Change committed as 198205

[Isaac (away), 2013-05-03 22:00:38]

Some comments inline.

I'm a confused by the use of ready_cond in the existing code.  Could you ping me
off-line before committing?

https://codereview.chromium.org/14759006/diff/19001/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/14759006/diff/19001/gclient_utils.py#newcode326
gclient_utils.py:326: GCLIENT_CHILDREN = []
I think set() would be a better data structure.

https://codereview.chromium.org/14759006/diff/19001/gclient_utils.py#newcode359
gclient_utils.py:359: if GCLIENT_CHILDREN:
Should this filter based on k.poll()?

https://codereview.chromium.org/14759006/diff/19001/gclient_utils.py#newcode719
gclient_utils.py:719: self.daemon = True
nice!

https://codereview.chromium.org/14759006/diff/19001/gclient_utils.py#newcode728
gclient_utils.py:728: logging.info('Caught KeyboardInterrupt in thread %s' %
self.item.name)
Use logger formatting, i.e.:

logging.info('Caught KeyboardInterrupt in thread %s', self.item.name)

https://codereview.chromium.org/14759006/diff/19001/gclient_utils.py#newcode738
gclient_utils.py:738: logging.info('_Worker.run(%s) done' % self.item.name)
ditto, don't format in logging strings.

[Isaac (away), 2013-05-03 22:02:08]

Looks like I sent this too late -- could you please create follow up patch
addressing comments?  Thanks.