Commit 8cf5c3d2 authored by nednguyen's avatar nednguyen Committed by Commit bot

[Telemtry] Create a temp file for storing wpr's stdout & stderr.

Previously, webpagereplay stores all the stdout & stder of the wpr subprocess to
a fix log file path location. It then parses the log lines to get replay server
port  assignment. When there are multiple telemetry processes running in parallel,
this cause a concurrency problem.

This patch addresses the issue by create a temp location for the log file path.
This is to make sure that different telemetry processes will write & read to different
log files.

BUG=469296

Review URL: https://codereview.chromium.org/1025553008

Cr-Commit-Position: refs/heads/master@{#322247}
parent 40fa1747
...@@ -10,6 +10,7 @@ import re ...@@ -10,6 +10,7 @@ import re
import signal import signal
import subprocess import subprocess
import sys import sys
import tempfile
import urllib import urllib
from telemetry.core import exceptions from telemetry.core import exceptions
...@@ -17,8 +18,6 @@ from telemetry.core import util ...@@ -17,8 +18,6 @@ from telemetry.core import util
_REPLAY_DIR = os.path.join( _REPLAY_DIR = os.path.join(
util.GetChromiumSrcDir(), 'third_party', 'webpagereplay') util.GetChromiumSrcDir(), 'third_party', 'webpagereplay')
_LOG_FILE_PATH = os.path.join(
util.GetChromiumSrcDir(), 'webpagereplay_logs', 'logs.txt')
class ReplayError(Exception): class ReplayError(Exception):
...@@ -72,11 +71,14 @@ class ReplayServer(object): ...@@ -72,11 +71,14 @@ class ReplayServer(object):
replay_options: an iterable of options strings to forward to replay.py. replay_options: an iterable of options strings to forward to replay.py.
""" """
self.archive_path = archive_path self.archive_path = archive_path
self._log_file_path = _LOG_FILE_PATH
self._replay_host = replay_host self._replay_host = replay_host
self._use_dns_server = dns_port is not None self._use_dns_server = dns_port is not None
self._started_ports = {} # a dict such as {'http': 80, 'https': 443} self._started_ports = {} # a dict such as {'http': 80, 'https': 443}
# A temporary path for storing stdout & stderr of the webpagereplay
# subprocess.
self._temp_log_file_path = None
replay_py = os.path.join(_REPLAY_DIR, 'replay.py') replay_py = os.path.join(_REPLAY_DIR, 'replay.py')
self._cmd_line = self._GetCommandLine( self._cmd_line = self._GetCommandLine(
replay_py, self._replay_host, http_port, https_port, dns_port, replay_py, self._replay_host, http_port, https_port, dns_port,
...@@ -120,16 +122,16 @@ class ReplayServer(object): ...@@ -120,16 +122,16 @@ class ReplayServer(object):
def _OpenLogFile(self): def _OpenLogFile(self):
"""Opens the log file for writing.""" """Opens the log file for writing."""
log_dir = os.path.dirname(self._log_file_path) log_dir = os.path.dirname(self._temp_log_file_path)
if not os.path.exists(log_dir): if not os.path.exists(log_dir):
os.makedirs(log_dir) os.makedirs(log_dir)
return open(self._log_file_path, 'w') return open(self._temp_log_file_path, 'w')
def _LogLines(self): def _LogLines(self):
"""Yields the log lines.""" """Yields the log lines."""
if not os.path.isfile(self._log_file_path): if not os.path.isfile(self._temp_log_file_path):
return return
with open(self._log_file_path) as f: with open(self._temp_log_file_path) as f:
for line in f: for line in f:
yield line yield line
...@@ -193,6 +195,7 @@ class ReplayServer(object): ...@@ -193,6 +195,7 @@ class ReplayServer(object):
""" """
is_posix = sys.platform.startswith('linux') or sys.platform == 'darwin' is_posix = sys.platform.startswith('linux') or sys.platform == 'darwin'
logging.debug('Starting Web-Page-Replay: %s', self._cmd_line) logging.debug('Starting Web-Page-Replay: %s', self._cmd_line)
self._CreateTempLogFilePath()
with self._OpenLogFile() as log_fh: with self._OpenLogFile() as log_fh:
self.replay_process = subprocess.Popen( self.replay_process = subprocess.Popen(
self._cmd_line, stdout=log_fh, stderr=subprocess.STDOUT, self._cmd_line, stdout=log_fh, stderr=subprocess.STDOUT,
...@@ -211,6 +214,12 @@ class ReplayServer(object): ...@@ -211,6 +214,12 @@ class ReplayServer(object):
def StopServer(self): def StopServer(self):
"""Stop Web Page Replay.""" """Stop Web Page Replay."""
try:
self._StopReplayProcess()
finally:
self._CleanUpTempLogFilePath()
def _StopReplayProcess(self):
if not self.replay_process: if not self.replay_process:
return return
...@@ -250,6 +259,18 @@ class ReplayServer(object): ...@@ -250,6 +259,18 @@ class ReplayServer(object):
pass pass
self.replay_process.wait() self.replay_process.wait()
def _CreateTempLogFilePath(self):
assert self._temp_log_file_path is None
handle, self._temp_log_file_path = tempfile.mkstemp()
os.close(handle)
def _CleanUpTempLogFilePath(self):
assert self._temp_log_file_path
# TODO(nednguyen): print the content of the log file path to telemetry's
# output before clearing the file.
os.remove(self._temp_log_file_path)
self._temp_log_file_path = None
def __enter__(self): def __enter__(self):
"""Add support for with-statement.""" """Add support for with-statement."""
self.StartServer() self.StartServer()
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment