Explain why _ResetInterruptHandler is needed.

Explain why _ResetInterruptHandler is needed.

BUG=254572
Committed: https://crrev.com/153901d9ecb847a6eeda59d960425b482480a7fb
Cr-Commit-Position: refs/heads/master@{#298737}

[slamm, 2014-10-07 22:09:49]

Make the bug link https

[slamm, 2014-10-07 22:11:47]

slamm@chromium.org changed reviewers:
+ nednguyen@google.com, tonyg@chromium.org

[nednguyen, 2014-10-07 22:18:30]

lgtm

https://codereview.chromium.org/637123002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/webpagereplay.py (right):

https://codereview.chromium.org/637123002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/webpagereplay.py:212:
preexec_fn=(_ResetInterruptHandler if is_posix else None))
+1 for cleanup the kwargs.

[slamm, 2014-10-07 23:53:05]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-07 23:54:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637123002/20001

[commit-bot: I haz the power, 2014-10-08 01:07:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 01:07:58]

Failed to apply patch for tools/telemetry/telemetry/core/webpagereplay.py:
While running git apply --index -3 -p1;
  error: patch failed: tools/telemetry/telemetry/core/webpagereplay.py:16
  Falling back to three-way merge...
  Applied patch to 'tools/telemetry/telemetry/core/webpagereplay.py' with
conflicts.
  U tools/telemetry/telemetry/core/webpagereplay.py

Patch:       tools/telemetry/telemetry/core/webpagereplay.py
Index: tools/telemetry/telemetry/core/webpagereplay.py
diff --git a/tools/telemetry/telemetry/core/webpagereplay.py
b/tools/telemetry/telemetry/core/webpagereplay.py
index
19a85bcf1d8764f186618d1c4e130783e9b1ce49..0461c93476a93e657f1f521354a189a7fa192ce0
100644
--- a/tools/telemetry/telemetry/core/webpagereplay.py
+++ b/tools/telemetry/telemetry/core/webpagereplay.py
@@ -16,6 +16,7 @@ import subprocess
 import sys
 import urllib
 
+from telemetry.core import platform
 from telemetry.core import util
 
 _CHROME_SRC_DIR = os.path.abspath(os.path.join(
@@ -26,15 +27,6 @@ LOG_PATH = os.path.join(
     _CHROME_SRC_DIR, 'webpagereplay_logs', 'logs.txt')
 
 
-# Signal masks on Linux are inherited from parent processes.  If anything
-# invoking us accidentally masks SIGINT (e.g. by putting a process in the
-# background from a shell script), sending a SIGINT to the child will fail
-# to terminate it.  Running this signal handler before execing should fix that
-# problem.
-def ResetInterruptHandler():
-  signal.signal(signal.SIGINT, signal.SIG_DFL)
-
-
 class ReplayError(Exception):
   """Catch-all exception for the module."""
   pass
@@ -211,14 +203,13 @@ class ReplayServer(object):
     cmd_line = [sys.executable, self.replay_py]
     cmd_line.extend(self.replay_options)
     cmd_line.append(self.archive_path)
+    is_posix = platform.GetHostPlatform().GetOSName() in ('linux', 'mac')
 
     logging.debug('Starting Web-Page-Replay: %s', cmd_line)
     with self._OpenLogFile() as log_fh:
-      kwargs = {'stdout': log_fh, 'stderr': subprocess.STDOUT}
-      if sys.platform.startswith('linux') or sys.platform == 'darwin':
-        kwargs['preexec_fn'] = ResetInterruptHandler
-      self.replay_process = subprocess.Popen(cmd_line, **kwargs)
-
+      self.replay_process = subprocess.Popen(
+          cmd_line, stdout=log_fh, stderr=subprocess.STDOUT,
+          preexec_fn=(_ResetInterruptHandler if is_posix else None))
     try:
       util.WaitFor(self._IsStarted, 30)
       return (
@@ -284,3 +275,22 @@ class ReplayServer(object):
     url = '%s://%s:%s/%s' % (
         protocol, self._replay_host, self._started_ports[protocol], url_path)
     return urllib.urlopen(url, proxies={})
+
+def _ResetInterruptHandler():
+  """Reset the interrupt handler back to the default.
+
+  The replay process is stopped gracefully by making an HTTP request
+  ('web-page-replay-command-exit'). The graceful exit is important for
+  restoring the DNS configuration. If the HTTP request fails, the fallback
+  is to send SIGINT to the process.
+
+  On posix system, running this function before starting replay fixes a
+  bug that shows up when Telemetry is run as a background command from a
+  script. https://crbug.com/254572.
+
+  Background: Signal masks on Linux are inherited from parent
+  processes. If anything invoking us accidentally masks SIGINT
+  (e.g. by putting a process in the background from a shell script),
+  sending a SIGINT to the child will fail to terminate it.
+  """
+  signal.signal(signal.SIGINT, signal.SIG_DFL)

[commit-bot: I haz the power, 2014-10-08 01:08:42]

The CQ bit was unchecked by commit-bot@chromium.org

[slamm, 2014-10-08 16:14:37]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-08 16:15:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637123002/40001

[commit-bot: I haz the power, 2014-10-08 17:21:21]

Committed patchset #3 (id:40001) as 442e1d876b43c790fb9dd6b375e7cae7e81b6135

[commit-bot: I haz the power, 2014-10-08 17:22:01]

Patchset 3 (id:??) landed as
https://crrev.com/153901d9ecb847a6eeda59d960425b482480a7fb
Cr-Commit-Position: refs/heads/master@{#298737}