Add small util for killing hanging processes.

Add small util for killing hanging processes.

This will be used on the buildbot to kill any hanging dart or browser
processes. We will print the command line that resulted in the hang to
enable us to track over time what is causing us to hang (at least for
the hanging dart processes).

R=kustermann@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26778

[kustermann, 2013-08-27 08:42:21]

Only small comments.

I've one concern: We call the external tools now multiple times (and that takes
a lot of time). I'd do something like that:

def ShouldKillProcess: ...
processes = ListProcesses()
processes_to_kill = filter(processes, ShouldKillProcess)
process_infos = map(processes_to_kill, GetProcessInfo)
num_processes_killed = 0
for pid, info in zip(processes_to_kill, process_infos):
  if KillProcess(pid):
    print("Killed %s" % info)
    num_processes_killed += 1

return num_processes_killed

This way we list the processes only once.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py
File tools/task_kill.py (right):

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode23
tools/task_kill.py:23: POSIX_LIST = 'ps -e -o comm,pid'
Please add a comment, which gives an example output. Same for windows.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode42
tools/task_kill.py:42: parser.add_option("--kill_browsers", default=False,
Why the difference in default values?

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode49
tools/task_kill.py:49: # PROCESS_NAME WHITESPACE* PID
Don't know if this is working. Often on mac and windows we've spaces in
directory/executable names.

I'd prefer a regexp like this:
line = re.compile('^(.+)\s+([0-9]+)$')

It's better for documentation and probably more reliable.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode59
tools/task_kill.py:59: lines.pop(0)
Do we have this header on windows and linux?

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode63
tools/task_kill.py:63: results.append(split[1])
see comment above.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode84
tools/task_kill.py:84: print("  Command line: %s" % line)
Same comments as above.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode88
tools/task_kill.py:88: os.kill(int(pid), signal.SIGTERM);
What if SIGTERM is ignored? We should really make sure, that the hanging
processes are gone.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode116
tools/task_kill.py:116: print("Killed pid: %s" % pid)
You don't know if it was successfull or not at this place (maybe we actually
didn't kill it)

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode118
tools/task_kill.py:118: print("  Nothing to kill")
We could print this out multiple times (for every Kill() invocation) -- which is
strange.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode119
tools/task_kill.py:119: return len(pids)
We should return here how many we actually killed.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode124
tools/task_kill.py:124: status += Kill('iexplore')
"status" is not the correct name. We should call it 'num_killed' or something
like that.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode125
tools/task_kill.py:125: status += Kill('Safari')
firefox is probably uppercase on mac. We should make these names os dependend.
(Then the GetExecutableName() function is also obsolete)

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode135
tools/task_kill.py:135: os_name = utils.GuessOS()
Don't do it here. Just do assign it at the module scope.

[ricow1, 2013-08-27 11:45:24]

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py
File tools/task_kill.py (right):

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode23
tools/task_kill.py:23: POSIX_LIST = 'ps -e -o comm,pid'
On 2013/08/27 08:42:22, kustermann wrote:
> Please add a comment, which gives an example output. Same for windows.

This has been moved into the platform specific functions, with sample output

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode42
tools/task_kill.py:42: parser.add_option("--kill_browsers", default=False,
On 2013/08/27 08:42:22, kustermann wrote:
> Why the difference in default values?

Because if people use it locally I don't want to kill their browsers by default

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode49
tools/task_kill.py:49: # PROCESS_NAME WHITESPACE* PID
On 2013/08/27 08:42:22, kustermann wrote:
> Don't know if this is working. Often on mac and windows we've spaces in
> directory/executable names.
> 
> I'd prefer a regexp like this:
> line = re.compile('^(.+)\s+([0-9]+)$')
> 
> It's better for documentation and probably more reliable.
I changed the order now and just return the last part of the string on posix, on
windows we actually get it for the explicit binary name

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode59
tools/task_kill.py:59: lines.pop(0)
On 2013/08/27 08:42:22, kustermann wrote:
> Do we have this header on windows and linux?

Yes we did, but not with the new command

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode63
tools/task_kill.py:63: results.append(split[1])
On 2013/08/27 08:42:22, kustermann wrote:
> see comment above.

Done.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode84
tools/task_kill.py:84: print("  Command line: %s" % line)
On 2013/08/27 08:42:22, kustermann wrote:
> Same comments as above.

Done.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode88
tools/task_kill.py:88: os.kill(int(pid), signal.SIGTERM);
On 2013/08/27 08:42:22, kustermann wrote:
> What if SIGTERM is ignored? We should really make sure, that the hanging
> processes are gone.

Done.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode88
tools/task_kill.py:88: os.kill(int(pid), signal.SIGTERM);
On 2013/08/27 08:42:22, kustermann wrote:
> What if SIGTERM is ignored? We should really make sure, that the hanging
> processes are gone.

Done.

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode118
tools/task_kill.py:118: print("  Nothing to kill")
On 2013/08/27 08:42:22, kustermann wrote:
> We could print this out multiple times (for every Kill() invocation) -- which
is
> strange.
That is what I want, the log will explicitly show that we try to kill the
process

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode119
tools/task_kill.py:119: return len(pids)
On 2013/08/27 08:42:22, kustermann wrote:
> We should return here how many we actually killed.

well, that is hard to figure out without listing again, kill the main chrome
process for example will take down the render processes as well

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode124
tools/task_kill.py:124: status += Kill('iexplore')
On 2013/08/27 08:42:22, kustermann wrote:
> "status" is not the correct name. We should call it 'num_killed' or something
> like that. 
Well, it is the return status :-)

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode125
tools/task_kill.py:125: status += Kill('Safari')
On 2013/08/27 08:42:22, kustermann wrote:
> firefox is probably uppercase on mac. We should make these names os dependend.
> (Then the GetExecutableName() function is also obsolete)

No it is not, but chrome is

https://codereview.chromium.org/23549002/diff/3001/tools/task_kill.py#newcode135
tools/task_kill.py:135: os_name = utils.GuessOS()
On 2013/08/27 08:42:22, kustermann wrote:
> Don't do it here. Just do assign it at the module scope.

Done.

[kustermann, 2013-08-27 12:33:14]

LGTM

https://codereview.chromium.org/23549002/diff/8001/tools/task_kill.py
File tools/task_kill.py (right):

https://codereview.chromium.org/23549002/diff/8001/tools/task_kill.py#newcode35
tools/task_kill.py:35: 'win32': 'wmic process where Processid=%s get
CommandLine',
Don't you need to quote this 'where Processid='?

https://codereview.chromium.org/23549002/diff/8001/tools/task_kill.py#newcode131
tools/task_kill.py:131: shell=True)
if you do this, you have to do
p.communicate()

https://codereview.chromium.org/23549002/diff/8001/tools/task_kill.py#newcode147
tools/task_kill.py:147: print("  Nothing to kill")
"Nothing to kill" -> "No %s processes found." % name

https://codereview.chromium.org/23549002/diff/8001/tools/task_kill.py#newcode167
tools/task_kill.py:167: status = status + KillBrowsers()
+= instead

[ricow1, 2013-08-28 11:08:07]

Committed patchset #4 manually as r26778 (presubmit successful).