Bug 38248 - webkitpy: ScriptError('Failed to run "[u\'taskkill.exe\', u\'/f\', u\'/im\', u\'httpd.exe\']" exit_code: 128',)
Summary: webkitpy: ScriptError('Failed to run "[u\'taskkill.exe\', u\'/f\', u\'/im\', ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows Vista
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-28 01:11 PDT by Fumitoshi Ukai
Modified: 2010-04-28 11:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.25 KB, patch)
2010-04-28 02:33 PDT, Eric Seidel (no email)
jorlow: review+
jorlow: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-04-28 01:11:01 PDT
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.
Comment 1 Eric Seidel (no email) 2010-04-28 01:14:01 PDT
Thank you for filing this!  I'm working on a fix now.
Comment 2 Eric Seidel (no email) 2010-04-28 02:23:34 PDT
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.
Comment 3 Eric Seidel (no email) 2010-04-28 02:33:28 PDT
Created attachment 54539 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-04-28 02:34:33 PDT
I think the problem here was that killall "httpd" is never going to succeed on windows since we use lighttpd. :)
Comment 5 Fumitoshi Ukai 2010-04-28 02:40:54 PDT
(In reply to comment #3)
> Created an attachment (id=54539) [details]
> Patch

LGTM.
Thanks for fixing!
Comment 6 Eric Seidel (no email) 2010-04-28 02:44:25 PDT
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.
Comment 7 Fumitoshi Ukai 2010-04-28 02:54:53 PDT
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?
Comment 8 Jeremy Orlow 2010-04-28 02:55:54 PDT
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
Comment 9 Jeremy Orlow 2010-04-28 02:56:50 PDT
Comment on attachment 54539 [details]
Patch

Wait...didn't see Ukai's comment.
Comment 10 Jeremy Orlow 2010-04-28 03:06:09 PDT
Comment on attachment 54539 [details]
Patch

r=me.  Ukia, please fix while landing.
Comment 11 Fumitoshi Ukai 2010-04-28 03:09:25 PDT
Committed r58397: <http://trac.webkit.org/changeset/58397>
Comment 12 Eric Seidel (no email) 2010-04-28 11:49:36 PDT
            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.