Don't force kill apache, and use the Apache-supplied PID for web server cleanup.

SIGKILL was causing Apache to leave semaphores hanging around, leading to resource starvation and Apache failures on the bots (see also http://rackerhacker.com/2007/08/24/apache-no-space-left-on-device-couldnt-create-accept-lock/).

Using the PID returned from Popen wasn't actually cleaning up anything, causing Apache processes to linger until the next run. Also change the shutdown method to just take a pid, since that's all it uses from the process object anyhow.

TEST=After running layout_test_wrapper.py (like http://chrome-buildbot.corp.google.com:8016/builders/Webkit%20Linux%2064%20(V8-Latest)/builds/5080/steps/webkit_tests/logs/stdio), there are no lingering chrome-bot semaphores (ipcs -s | grep chrome) or apache processes (ps aux | grep apache2)

Review URL: http://codereview.chromium.org/491025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34307 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 36 insertions, 23 deletions

diff --git a/webkit/tools/layout_tests/layout_package/apache_http_server.py b/webkit/tools/layout_tests/layout_package/apache_http_server.py
index 8b75872..c8ab7ad 100644
--- a/webkit/tools/layout_tests/layout_package/apache_http_server.py
+++ b/webkit/tools/layout_tests/layout_package/apache_http_server.py
@@ -32,7 +32,13 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
 
     # The upstream .conf file assumed the existence of /tmp/WebKit for placing
     # apache files like the lock file there.
-    path_utils.MaybeMakeDirectory(os.path.join("/tmp", "WebKit"))
+    self._runtime_path = os.path.join("/tmp", "WebKit")
+    path_utils.MaybeMakeDirectory(self._runtime_path)
+
+    # The PID returned when Apache is started goes away (due to dropping
+    # privileges?). The proper controlling PID is written to a file in the
+    # apache runtime directory.
+    self._pid_file = os.path.join(self._runtime_path, 'httpd.pid')
 
     test_dir = path_utils.PathFromBase('third_party', 'WebKit',
         'LayoutTests')
@@ -120,4 +126,7 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
   def Stop(self):
     """Stops the apache http server."""
     logging.debug("Shutting down any running http servers")
-    path_utils.ShutDownHTTPServer(self._httpd_proc)
+    httpd_pid = None
+    if os.path.exists(self._pid_file):
+      httpd_pid = int(open(self._pid_file).readline())
+    path_utils.ShutDownHTTPServer(httpd_pid)
diff --git a/webkit/tools/layout_tests/layout_package/http_server.py b/webkit/tools/layout_tests/layout_package/http_server.py
index 898bfae..23ef22a 100755
--- a/webkit/tools/layout_tests/layout_package/http_server.py
+++ b/webkit/tools/layout_tests/layout_package/http_server.py
@@ -231,7 +231,10 @@ class Lighttpd(http_server_base.HttpServerBase):
     if not force and not self.IsRunning():
       return
 
-    path_utils.ShutDownHTTPServer(self._process)
+    httpd_pid = None
+    if self._process:
+      httpd_pid = self._process.pid
+    path_utils.ShutDownHTTPServer(httpd_pid)
 
     if self._process:
       self._process.wait()
diff --git a/webkit/tools/layout_tests/layout_package/path_utils.py b/webkit/tools/layout_tests/layout_package/path_utils.py
index 3d0c56c..7099df2 100644
--- a/webkit/tools/layout_tests/layout_package/path_utils.py
+++ b/webkit/tools/layout_tests/layout_package/path_utils.py
@@ -355,8 +355,8 @@ def LayoutTestHelperPath(target):
 def FuzzyMatchPath():
   return platform_utils.FuzzyMatchPath()
 
-def ShutDownHTTPServer(server_process):
-  return platform_utils.ShutDownHTTPServer(server_process)
+def ShutDownHTTPServer(server_pid):
+  return platform_utils.ShutDownHTTPServer(server_pid)
 
 def KillAllTestShells():
   platform_utils.KillAllTestShells()
diff --git a/webkit/tools/layout_tests/layout_package/platform_utils_linux.py b/webkit/tools/layout_tests/layout_package/platform_utils_linux.py
index 4c09676..fac1b97 100644
--- a/webkit/tools/layout_tests/layout_package/platform_utils_linux.py
+++ b/webkit/tools/layout_tests/layout_package/platform_utils_linux.py
@@ -124,24 +124,25 @@ def FuzzyMatchPath():
   """Return the path to the fuzzy matcher binary."""
   return path_utils.PathFromBase('third_party', 'fuzzymatch', 'fuzzymatch')
 
-def ShutDownHTTPServer(server_process):
+def ShutDownHTTPServer(server_pid):
   """Shut down the lighttpd web server. Blocks until it's fully shut down.
 
   Args:
-    server_process: The subprocess object representing the running server
+    server_pid: The process ID of the running server.
   """
-  # server_process is not set when "http_server.py stop" is run manually.
-  if server_process is None:
-    # TODO(mmoss) This isn't ideal, since it could conflict with lighttpd
-    # processes not started by http_server.py, but good enough for now.
+  # server_pid is not set when "http_server.py stop" is run manually.
+  if server_pid is None:
+    # This isn't ideal, since it could conflict with web server processes not
+    # started by http_server.py, but good enough for now.
     null = open("/dev/null");
-    subprocess.call(['killall', '-KILL', '-u', os.getenv('USER'), 'lighttpd'],
+    subprocess.call(['killall', '-TERM', '-u', os.getenv('USER'), 'lighttpd'],
                     stderr=null)
-    subprocess.call(['killall', '-KILL', '-u', os.getenv('USER'), 'apache2'],
+    subprocess.call(['killall', '-TERM', '-u', os.getenv('USER'), 'apache2'],
                     stderr=null)
     null.close()
   else:
-    os.kill(server_process.pid, signal.SIGKILL)
+    os.kill(server_pid, signal.SIGTERM)
+    #TODO(mmoss) Maybe throw in a SIGKILL just to be sure?
 
 def KillProcess(pid):
   """Forcefully kill the process.
diff --git a/webkit/tools/layout_tests/layout_package/platform_utils_mac.py b/webkit/tools/layout_tests/layout_package/platform_utils_mac.py
index 116375e..e0a78ee 100644
--- a/webkit/tools/layout_tests/layout_package/platform_utils_mac.py
+++ b/webkit/tools/layout_tests/layout_package/platform_utils_mac.py
@@ -101,14 +101,14 @@ def LigHTTPdPHPPath():
   return path_utils.PathFromBase('third_party', 'lighttpd', 'mac', 'bin',
                                  'php-cgi')
 
-def ShutDownHTTPServer(server_process):
+def ShutDownHTTPServer(server_pid):
   """Shut down the lighttpd web server. Blocks until it's fully shut down.
 
     Args:
-      server_process: The subprocess object representing the running server
+      server_pid: The process ID of the running server.
   """
-  # server_process is not set when "http_server.py stop" is run manually.
-  if server_process is None:
+  # server_pid is not set when "http_server.py stop" is run manually.
+  if server_pid is None:
     # TODO(mmoss) This isn't ideal, since it could conflict with lighttpd
     # processes not started by http_server.py, but good enough for now.
 
@@ -118,13 +118,13 @@ def ShutDownHTTPServer(server_process):
     #   killall: illegal option -- T
     # Use of the earlier -TERM placement is just fine on 10.5.
     null = open("/dev/null");
-    subprocess.call(['killall', '-KILL', '-u', os.getenv('USER'), 'lighttpd'],
+    subprocess.call(['killall', '-TERM', '-u', os.getenv('USER'), 'lighttpd'],
                     stderr=null)
-    subprocess.call(['killall', '-KILL', '-u', os.getenv('USER'), 'httpd'],
+    subprocess.call(['killall', '-TERM', '-u', os.getenv('USER'), 'httpd'],
                     stderr=null)
     null.close()
   else:
-    os.kill(server_process.pid, signal.SIGKILL)
+    os.kill(server_pid, signal.SIGTERM)
 
 def KillProcess(pid):
   """Forcefully kill the process.
diff --git a/webkit/tools/layout_tests/layout_package/platform_utils_win.py b/webkit/tools/layout_tests/layout_package/platform_utils_win.py
index 7017ad6..746407f 100644
--- a/webkit/tools/layout_tests/layout_package/platform_utils_win.py
+++ b/webkit/tools/layout_tests/layout_package/platform_utils_win.py
@@ -118,11 +118,11 @@ def LigHTTPdPHPPath():
   return path_utils.PathFromBase('third_party', 'lighttpd', 'win', 'php5',
                                  'php-cgi.exe')
 
-def ShutDownHTTPServer(server_process):
+def ShutDownHTTPServer(server_pid):
   """Shut down the lighttpd web server. Blocks until it's fully shut down.
 
   Args:
-    server_process: The subprocess object representing the running server.
+    server_pid: The process ID of the running server.
         Unused in this implementation of the method.
   """
   subprocess.Popen(('taskkill.exe', '/f', '/im', 'LightTPD.exe'),