Remove the overridden _stop_running_server method in WPTServe.

Remove the overridden _stop_running_server method in WPTServe.

Background: When support for wptserve was originally added [1],
WPTServe had a _stop_running_server to override the version in
ServerBase, which tries to kill the existing process with
Executive.interrupt instead of  Executive.kill_process on non-Windows
platforms.

Later this method was changed in http://crrev.com/2511013002 and
now when interrupt fails to kill the process, there's an infinite
loop which results in the job stalling for days.

But, I'm not sure why it was necessary to use Executive.interrupt
rather than Executive.kill_process. If the overridden method is removed,
then ServerBase._stop_running_server would be used, which is:

    def _stop_running_server(self):
        self._wait_for_action(self._check_and_kill)
        if self._filesystem.exists(self._pid_file):
            self._filesystem.remove(self._pid_file)

[1] https://chromium.googlesource.com/chromium/src/+/0129094b0ad431a2aee9cf5ea9225037283f3c37/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py

Note, the actual change to behavior here is in webkitpy/layout_tests/servers/wptserve.py -- the rest of the changes are related to unit testing; I could also put those changes in a separate CL.

BUG=669802
Review-Url: https://codereview.chromium.org/2624093003
Cr-Commit-Position: refs/heads/master@{#443083}
Committed: https://chromium.googlesource.com/chromium/src/+/66e610ba7ddabd8f30757b0378aa4185b893cdf4

[qyearsley, 2017-01-11 23:23:02]

Description was changed from

==========
Remove the overridden _stop_running_server method in WPTServe.

Background: When support for wptserve was originally added [1],
WPTServe had a _stop_running_server to override the version in
ServerBase.

[1]
https://chromium.googlesource.com/chromium/src/+/0129094b0ad431a2aee9cf5ea922...

BUG=669802
==========

to

==========
Remove the overridden _stop_running_server method in WPTServe.

Background: When support for wptserve was originally added [1],
WPTServe had a _stop_running_server to override the version in
ServerBase, which tries to kill the existing process with
Executive.interrupt instead of  Executive.kill_process on non-Windows
platforms.

Later this method was changed in http://crrev.com/2511013002 and
now when interrupt fails to kill the process, there's an infinite
loop which results in the job stalling for days.

But, I'm not sure why it was necessary to use Executive.interrupt
rather than Executive.kill_process. If the overridden method is removed,
then ServerBase._stop_running_server would be used, which is:

    def _stop_running_server(self):
        self._wait_for_action(self._check_and_kill)
        if self._filesystem.exists(self._pid_file):
            self._filesystem.remove(self._pid_file)

[1]
https://chromium.googlesource.com/chromium/src/+/0129094b0ad431a2aee9cf5ea922...

Note, the actual change to behavior here is in
webkitpy/layout_tests/servers/wptserve.py -- the rest of the changes are related
to unit testing; I could also put those changes in a separate CL.

BUG=669802
==========

[qyearsley, 2017-01-11 23:23:02]

qyearsley@chromium.org changed reviewers:
+ tkent@chromium.org

[qyearsley, 2017-01-11 23:23:13]

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

[commit-bot: I haz the power, 2017-01-11 23:24:01]

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

[qyearsley, 2017-01-11 23:52:23]

https://codereview.chromium.org/2624093003/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py
(left):

https://codereview.chromium.org/2624093003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:52:
self._keep_process_reference = True
This is also unused, but removing this line is not necessary in this CL I think.

[tkent, 2017-01-12 00:02:25]

lgtm

[Dirk Pranke, 2017-01-12 00:11:15]

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

[Dirk Pranke, 2017-01-12 00:11:15]

lgtm

[qyearsley, 2017-01-12 00:13:02]

The CQ bit was unchecked by qyearsley@chromium.org

[qyearsley, 2017-01-12 00:13:03]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2017-01-12 00:13:38]

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, 2017-01-12 00:35:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484179983829380, "parent_rev":
"6d75acffb17270bac23b8b469f9f1d526dad53d2", "commit_rev":
"66e610ba7ddabd8f30757b0378aa4185b893cdf4"}

[commit-bot: I haz the power, 2017-01-12 00:36:15]

Description was changed from

==========
Remove the overridden _stop_running_server method in WPTServe.

Background: When support for wptserve was originally added [1],
WPTServe had a _stop_running_server to override the version in
ServerBase, which tries to kill the existing process with
Executive.interrupt instead of  Executive.kill_process on non-Windows
platforms.

Later this method was changed in http://crrev.com/2511013002 and
now when interrupt fails to kill the process, there's an infinite
loop which results in the job stalling for days.

But, I'm not sure why it was necessary to use Executive.interrupt
rather than Executive.kill_process. If the overridden method is removed,
then ServerBase._stop_running_server would be used, which is:

    def _stop_running_server(self):
        self._wait_for_action(self._check_and_kill)
        if self._filesystem.exists(self._pid_file):
            self._filesystem.remove(self._pid_file)

[1]
https://chromium.googlesource.com/chromium/src/+/0129094b0ad431a2aee9cf5ea922...

Note, the actual change to behavior here is in
webkitpy/layout_tests/servers/wptserve.py -- the rest of the changes are related
to unit testing; I could also put those changes in a separate CL.

BUG=669802
==========

to

==========
Remove the overridden _stop_running_server method in WPTServe.

Background: When support for wptserve was originally added [1],
WPTServe had a _stop_running_server to override the version in
ServerBase, which tries to kill the existing process with
Executive.interrupt instead of  Executive.kill_process on non-Windows
platforms.

Later this method was changed in http://crrev.com/2511013002 and
now when interrupt fails to kill the process, there's an infinite
loop which results in the job stalling for days.

But, I'm not sure why it was necessary to use Executive.interrupt
rather than Executive.kill_process. If the overridden method is removed,
then ServerBase._stop_running_server would be used, which is:

    def _stop_running_server(self):
        self._wait_for_action(self._check_and_kill)
        if self._filesystem.exists(self._pid_file):
            self._filesystem.remove(self._pid_file)

[1]
https://chromium.googlesource.com/chromium/src/+/0129094b0ad431a2aee9cf5ea922...

Note, the actual change to behavior here is in
webkitpy/layout_tests/servers/wptserve.py -- the rest of the changes are related
to unit testing; I could also put those changes in a separate CL.

BUG=669802

Review-Url: https://codereview.chromium.org/2624093003
Cr-Commit-Position: refs/heads/master@{#443083}
Committed:
https://chromium.googlesource.com/chromium/src/+/66e610ba7ddabd8f30757b0378aa...
==========

[commit-bot: I haz the power, 2017-01-12 00:36:16]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/66e610ba7ddabd8f30757b0378aa...

[Marijn Kruisselbrink, 2017-01-19 19:52:15]

After this change for me trying to run tests locally no longer kills wptserve,
i.e. if I run run-webkit-tests external/wpt/some-wpt-test.html twice in a row,
the second run fails because wptserve from the first run is still running.
Reverting this CL and wptserve gets killed correctly. Not sure if this is
something special about my local setup?

[qyearsley, 2017-01-19 20:34:06]

On 2017/01/19 at 19:52:15, mek wrote:
> After this change for me trying to run tests locally no longer kills wptserve,
i.e. if I run run-webkit-tests external/wpt/some-wpt-test.html twice in a row,
the second run fails because wptserve from the first run is still running.
Reverting this CL and wptserve gets killed correctly. Not sure if this is
something special about my local setup?

I don't think that it's something special about your local setup, it's been
reproduced by others (including me); see http://crbug.com/682711.