Bug 38248 – webkitpy: ScriptError('Failed to run "[u\'taskkill.exe\', u\'/f\', u\'/im\', u\'httpd.exe\']" exit_code: 128',)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

Webkit (webkit.org) builder on chromium failed to kill httpd at the end of webkit tests, and left websocket server running.

http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/26489/steps/webkit_tests/logs/stdio

100428 00:54:31 run_webkit_tests.py:243  DEBUG flushing stdout
100428 00:54:31 run_webkit_tests.py:245  DEBUG flushing stderr
100428 00:54:31 run_webkit_tests.py:247  DEBUG stopping http server
Exception webkitpy.common.system.executive.ScriptError: ScriptError('Failed to run "[u\'taskkill.exe\', u\'/f\', u\'/im\', u\'httpd.exe\']" exit_code: 128',) in <bound method TestRunner.__del__ of <webkitpy.layout_tests.run_webkit_tests.TestRunner instance at 0x00E21918>> ignored

command timed out: 600 seconds without output, killing pid 18336
SIGKILL failed to kill process
using fake rc=-1
program finished with exit code -1

remoteFailed: [Failure instance: Traceback from remote host -- Traceback (most recent call last):
Failure: buildbot.slave.commands.TimeoutError: SIGKILL failed to kill process

Next webkit tests run would fail to remove output files of websocket before launching websocket server, because output file is used in running process.

[Eric Seidel (no email)]

Thank you for filing this!  I'm working on a fix now.

[Eric Seidel (no email)]

I'm not sure I understand.  Should the httpd server be running?  Are we failing to kill it?

The old code would have ignored this error.  So I'm fixing the new code to ignore it as well.

[Eric Seidel (no email)]

Created attachment 54539 [details]
Patch

[Eric Seidel (no email)]

I think the problem here was that killall "httpd" is never going to succeed on windows since we use lighttpd. :)

[Fumitoshi Ukai]

(In reply to comment #3)
> Created an attachment (id=54539) [details]
> Patch

LGTM.
Thanks for fixing!

[Eric Seidel (no email)]

The better fix would have probably been to stop trying to kill httpd when we know we didn't start it and then actually make the Executive.kill_all API throw an exception if no process exists.  But we can do that sort of change later.

[Fumitoshi Ukai]

Comment on attachment 54539 [details]
Patch


>          Reviewed by Darin Adler and Eric Seidel.
> diff --git a/WebKitTools/Scripts/webkitpy/common/system/executive.py b/WebKitTools/Scripts/webkitpy/common/system/executive.py
> index 5c1d28ca6f5ae45cebaad330018bc3b6c3c6cb1e..cb29834f4c970cecd63c105a373b2626a6391002 100644
> --- a/WebKitTools/Scripts/webkitpy/common/system/executive.py
> +++ b/WebKitTools/Scripts/webkitpy/common/system/executive.py
> @@ -161,17 +161,39 @@ class Executive(object):
>          return 2
>  
>      def kill_process(self, pid):
> +        """Attempts to kill the given pid.
> +        Will fail silently if pid does not exist or insufficient permisssions."""
>          if platform.system() == "Windows":
>              # According to http://docs.python.org/library/os.html
>              # os.kill isn't available on Windows.  However, when I tried it
>              # using Cygwin, it worked fine.  We should investigate whether
>              # we need this platform specific code here.
> -            subprocess.call(('taskkill.exe', '/f', '/pid', unicode(pid)),
> -                            stdin=open(os.devnull, 'r'),
> -                            stdout=subprocess.PIPE,
> -                            stderr=subprocess.PIPE)
> +            command = ["taskkill.exe", "/f", "/pid", pid]

str(pid) or unicode(pid) ?
are we sure pid is already string?

[Jeremy Orlow]

Comment on attachment 54539 [details]
Patch

Looks fine to me, Ukai did an unofficial review, and this is breaking the Chromium bots.  So r=me

[Jeremy Orlow]

Comment on attachment 54539 [details]
Patch

Wait...didn't see Ukai's comment.

[Jeremy Orlow]

Comment on attachment 54539 [details]
Patch

r=me.  Ukia, please fix while landing.

[Fumitoshi Ukai]

Committed r58397: <http://trac.webkit.org/changeset/58397>

[Eric Seidel (no email)]

            command = ["taskkill.exe", "/f", "/pid", str(pid)] 

is wrong.

run_command already knows how to convert arguments to strings.

I'll remove the "fix" in a later checkin.