Remove 'shell=True' in Popen and directly execute testserver.py

Remove 'shell=True' in Popen and directly execute testserver.py

In chrome_test_server_spawner.py, testserver process is created by
'subprocess.Popen(command, shell=True)', it will create 2 processes(http://goo.gl/oDYsn):
  (1) /bin/sh -c testserver.py ...
  (2) python testserver.py ...
but subprocess.Popen only returns the 1st process(pid), so when kill testserver with
Popen.kill(), only the 1st process is killed, the 2nd process(testserver) still
remains there.

This leads to two problems:
 (1) testserver failed to be killed for each case with the log:
     '"GET /kill HTTP/1.1" 500'
 (2) many teserver processes are there after each run, and the port is used,
     when starting a new run, will get the following error:
	 'socket.error: [Errno 98] Address already in use'

Remove the 'shell=True' and fix this issue.

BUG=
TEST=net_unittests_apk --gtest_filter=URLRequestTestHTTP.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158267

[Shouqun Liu, 2012-09-24 06:24:57]

Hi all, this patch fixes the issue caused by Popen
(http://stackoverflow.com/questions/6742635/popen-creates-an-extra-sh-process),
Using subprocess.Popen with "shell=True" leads to two processes created, but the
test server spawner only tracks the first one and can't actually kills the
testserver. Add 'exec' to fix this,
please review it.

[Isaac (away), 2012-09-24 06:49:56]

It is surprising to me that linux does not kill a child when the parent is
killed.  But in any case, shell=True is not necessary here so a better fix would
be to remove this and execute testserver.py way to fix this is to remove
shell=True and execute testserver.py directly.

[Isaac (away), 2012-09-24 06:51:08]

Sorry for the mangled grammer.

I suggest you remove shell=True and execute testserver.py directly.

[Shouqun Liu, 2012-09-24 07:07:29]

On 2012/09/24 06:51:08, Isaac wrote:
> Sorry for the mangled grammer.
> 
> I suggest you remove shell=True and execute testserver.py directly.

Hi, Isaac, I have updated this patch and removed the shell=True, verified OK,
thanks!

[Isaac (away), 2012-09-24 07:30:46]

http://codereview.chromium.org/10960050/diff/4003/build/android/pylib/chrome_...
File build/android/pylib/chrome_test_server_spawner.py (right):

http://codereview.chromium.org/10960050/diff/4003/build/android/pylib/chrome_...
build/android/pylib/chrome_test_server_spawner.py:216: args =
shlex.split(command)
Thanks!  We might be able to avoid shlex too, if command_line list has only one
argument per entry.  Do you think the following would work?

command = [os.path.join(...., 'testserver.py')] + self.command_line
logging.info('running: %s', command)
self.process = subprocess.Popen(command)

[Shouqun Liu, 2012-09-24 07:48:52]

On 2012/09/24 07:30:46, Isaac wrote:
>
http://codereview.chromium.org/10960050/diff/4003/build/android/pylib/chrome_...
> File build/android/pylib/chrome_test_server_spawner.py (right):
> 
>
http://codereview.chromium.org/10960050/diff/4003/build/android/pylib/chrome_...
> build/android/pylib/chrome_test_server_spawner.py:216: args =
> shlex.split(command)
> Thanks!  We might be able to avoid shlex too, if command_line list has only
one
> argument per entry.  Do you think the following would work?
> 
> command = [os.path.join(...., 'testserver.py')] + self.command_line
> logging.info('running: %s', command)
> self.process = subprocess.Popen(command)

Oh, yes, agree, patch updated

[Isaac (away), 2012-09-24 08:04:11]

LGTM

[bulach, 2012-09-24 10:28:02]

lgtm, one nit below. also, patch description will need updating :) thanks!

http://codereview.chromium.org/10960050/diff/5003/build/android/pylib/chrome_...
File build/android/pylib/chrome_test_server_spawner.py (right):

http://codereview.chromium.org/10960050/diff/5003/build/android/pylib/chrome_...
build/android/pylib/chrome_test_server_spawner.py:212: logging.info("Running:
%s", command)
nit: for consistency, use ' rather than "

[Shouqun Liu, 2012-09-24 11:19:32]

On 2012/09/24 10:28:02, bulach wrote:
> lgtm, one nit below. also, patch description will need updating :) thanks!
> 
>
http://codereview.chromium.org/10960050/diff/5003/build/android/pylib/chrome_...
> File build/android/pylib/chrome_test_server_spawner.py (right):
> 
>
http://codereview.chromium.org/10960050/diff/5003/build/android/pylib/chrome_...
> build/android/pylib/chrome_test_server_spawner.py:212: logging.info("Running:
> %s", command)
> nit: for consistency, use ' rather than "

fixed

[commit-bot: I haz the power, 2012-09-24 11:43:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10960050/9002

[commit-bot: I haz the power, 2012-09-24 13:38:42]

Change committed as 158267