Issue 663387 link

Starred by 1 user

Issue metadata

Status:	Archived
Owner:	jcliang@chromium.org
Closed:	Nov 2016
Cc:	hychao@chromium.org jcliang@chromium.org kcwu@chromium.org posciak@chromium.org █ nednguyen@chromium.org cychiang@chromium.org tbroch@chromium.org █ owenlin@chromium.org █ waihong@chromium.org ihf@chromium.org █ wuchengli@chromium.org
Components:	Tests
EstimatedDays:	----
NextAction:	----
OS:	Chrome
Pri:	2
Type:	Bug
VerifyIn-57 VerifyIn-58 VerifyIn-59 VerifyIn-60 VerifyIn-61

autotest: Calls to SetHTTPServerDirectories() were not properly cleaned up with StopAllLocalServers()

Project Member Reported by jcliang@chromium.org, Nov 8 2016

Issue description

A quick git grep shows that SetHTTPServerDirectories() [0] is used in ~40 places, but nearly none of them calls StopAllLocalServers() [1] to properly tear down the python web server. Without calling StopAllLocalServers() the python web server remains active after a test finishes. I've seen leaked web servers with high cpu usage several times. If another test is run alone with a high-cpu-usage leaked web server it may impact the test result (either power or performance -wise).

The categories of tests I found with this issue:

- video
- audio
- power
- graphics
- chameleon
- touch

[0]: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/core/platform.py?rcl=0&l=384
[1]: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/core/platform.py?rcl=0&l=413

Comment 1 by ihf@chromium.org, Nov 8 2016

Can we make the StopAll... calls automatic? Offer a construct like "with HttpServer ...". May also need to guard a test at the beginning. I think that could be done in "verify" jobs (lab internal) if it happens often.

Comment 2 by jcliang@chromium.org, Nov 8 2016

Cc: nednguyen@chromium.org

I guess we can add a wrapper that supports "with HttpServer..." in the chrome module in  autotest, and change most of the existing code to use it. This might be a good thing. I feel it's not trivial to me that SetHTTPServerDirectories() must be used with StopAllLocalServers().

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/2bfa5640825faf707213abd2bee03f684547a938

commit 2bfa5640825faf707213abd2bee03f684547a938
Author: Ricky Liang <jcliang@chromium.org>
Date: Wed Nov 09 02:18:37 2016

Call platform.StopAllLocalServers() in close()

This is to make sure the telemetry local servers, e.g. the one started by
platform.SetHTTPServerDirectories(), are closed properly.

BUG= chromium:663387 
TEST=Manually run several tests which use platform.SetHTTPServerDirectories()
     and make sure there's no process leak.

Change-Id: I63630b9df6898e34ee1114f66f22201d129498e5
Reviewed-on: https://chromium-review.googlesource.com/408835
Commit-Ready: Ilja H. Friedel <ihf@chromium.org>
Tested-by: Ricky Liang <jcliang@chromium.org>
Reviewed-by: Ilja H. Friedel <ihf@chromium.org>

[modify] https://crrev.com/2bfa5640825faf707213abd2bee03f684547a938/client/site_tests/graphics_WebGLPerformance/graphics_WebGLPerformance.py
[modify] https://crrev.com/2bfa5640825faf707213abd2bee03f684547a938/client/common_lib/cros/chrome.py
[modify] https://crrev.com/2bfa5640825faf707213abd2bee03f684547a938/client/site_tests/graphics_WebGLAquarium/graphics_WebGLAquarium.py
[modify] https://crrev.com/2bfa5640825faf707213abd2bee03f684547a938/client/cros/audio/audio_helper.py
[modify] https://crrev.com/2bfa5640825faf707213abd2bee03f684547a938/client/common_lib/cros/interactive_xmlrpc_server.py