- Revision
- 218055
- Author
- [email protected]
- Date
- 2017-06-10 12:51:13 -0700 (Sat, 10 Jun 2017)
Log Message
webkitpy: Reduce polling in ServerProcess
https://bugs.webkit.org/show_bug.cgi?id=173116
Reviewed by Ryosuke Niwa.
We should be smarter about polling. We do not need to poll every time a line is read from
stdout and stderr since lines are usually read from cached data. We should only poll
when extracting that cached data from stdout and stderr.
* Scripts/webkitpy/port/driver.py:
(Driver._read_block): Rely on output of the ServerProcess to detect a crash or a timeout
and on ServerProcess to poll the process if not data is available in stdout and stderr.
* Scripts/webkitpy/port/driver_unittest.py:
(DriverTest.test_read_block_crashed_process): Test that the Driver will handle a crashing
ServerProcess.
* Scripts/webkitpy/port/server_process.py:
(ServerProcess._read): Allow for data to be read from _output even if the process has
crashed. Only poll the process if data needs to be extracted from stdout or stderr.
* Scripts/webkitpy/port/server_process_mock.py:
(MockServerProcess): Add number_of_times_polled.
(MockServerProcess.poll): Increment number_of_times_polled.
(MockServerProcess.has_crashed): Poll before returning crash state.
(MockServerProcess.read_stdout_line): MockServerProcess should return None if it has crashed,
just like a ServerProcess would.
(MockServerProcess.read_stdout): Ditto.
* Scripts/webkitpy/port/server_process_unittest.py:
(TestServerProcess.test_basic): Use stdin.readline() instead of time.sleep() to prevent the
process from ending before stdout and stderr are read. This is the reason this test was flakey.
(TestServerProcess):
(TestServerProcess.test_process_crashing): Test that when a process crashes, data can be read until
the processes is polled.
(TestServerProcess.test_process_crashing_no_data): Test that when a process which has not output any
data to stdout and stderr crashes, ServerProcess._read(...) polls the process to detect the crash.
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (218054 => 218055)
--- trunk/Tools/ChangeLog 2017-06-10 19:20:47 UTC (rev 218054)
+++ trunk/Tools/ChangeLog 2017-06-10 19:51:13 UTC (rev 218055)
@@ -1,3 +1,39 @@
+2017-06-10 Jonathan Bedard <[email protected]>
+
+ webkitpy: Reduce polling in ServerProcess
+ https://bugs.webkit.org/show_bug.cgi?id=173116
+
+ Reviewed by Ryosuke Niwa.
+
+ We should be smarter about polling. We do not need to poll every time a line is read from
+ stdout and stderr since lines are usually read from cached data. We should only poll
+ when extracting that cached data from stdout and stderr.
+
+ * Scripts/webkitpy/port/driver.py:
+ (Driver._read_block): Rely on output of the ServerProcess to detect a crash or a timeout
+ and on ServerProcess to poll the process if not data is available in stdout and stderr.
+ * Scripts/webkitpy/port/driver_unittest.py:
+ (DriverTest.test_read_block_crashed_process): Test that the Driver will handle a crashing
+ ServerProcess.
+ * Scripts/webkitpy/port/server_process.py:
+ (ServerProcess._read): Allow for data to be read from _output even if the process has
+ crashed. Only poll the process if data needs to be extracted from stdout or stderr.
+ * Scripts/webkitpy/port/server_process_mock.py:
+ (MockServerProcess): Add number_of_times_polled.
+ (MockServerProcess.poll): Increment number_of_times_polled.
+ (MockServerProcess.has_crashed): Poll before returning crash state.
+ (MockServerProcess.read_stdout_line): MockServerProcess should return None if it has crashed,
+ just like a ServerProcess would.
+ (MockServerProcess.read_stdout): Ditto.
+ * Scripts/webkitpy/port/server_process_unittest.py:
+ (TestServerProcess.test_basic): Use stdin.readline() instead of time.sleep() to prevent the
+ process from ending before stdout and stderr are read. This is the reason this test was flakey.
+ (TestServerProcess):
+ (TestServerProcess.test_process_crashing): Test that when a process crashes, data can be read until
+ the processes is polled.
+ (TestServerProcess.test_process_crashing_no_data): Test that when a process which has not output any
+ data to stdout and stderr crashes, ServerProcess._read(...) polls the process to detect the crash.
+
2017-06-10 Andy Estes <[email protected]>
[QuickLook] PreviewLoader needs to check if its ResourceLoader has reached the terminal state before calling didReceiveResponse() and friends
Modified: trunk/Tools/Scripts/webkitpy/port/driver.py (218054 => 218055)
--- trunk/Tools/Scripts/webkitpy/port/driver.py 2017-06-10 19:20:47 UTC (rev 218054)
+++ trunk/Tools/Scripts/webkitpy/port/driver.py 2017-06-10 19:51:13 UTC (rev 218055)
@@ -552,7 +552,7 @@
out_seen_eof = False
asan_violation_detected = False
- while not self.has_crashed():
+ while True:
if out_seen_eof and (self.err_seen_eof or not wait_for_stderr_eof):
break
@@ -565,7 +565,8 @@
else:
out_line, err_line = self._server_process.read_either_stdout_or_stderr_line(deadline)
- if self._server_process.timed_out or self.has_crashed():
+ # ServerProcess returns None for time outs and crashes.
+ if out_line is None and err_line is None:
break
if out_line:
Modified: trunk/Tools/Scripts/webkitpy/port/driver_unittest.py (218054 => 218055)
--- trunk/Tools/Scripts/webkitpy/port/driver_unittest.py 2017-06-10 19:20:47 UTC (rev 218054)
+++ trunk/Tools/Scripts/webkitpy/port/driver_unittest.py 2017-06-10 19:51:13 UTC (rev 218055)
@@ -142,8 +142,28 @@
self.assertEqual(content_block.content_type, 'my_type')
self.assertEqual(content_block.encoding, 'none')
self.assertEqual(content_block.content_hash, 'foobar')
+ # We should only poll once for each line.
+ self.assertEqual(driver._server_process.number_of_times_polled, 4)
driver._server_process = None
+ def test_read_block_crashed_process(self):
+ port = TestWebKitPort()
+ driver = Driver(port, 0, pixel_tests=False)
+ driver._server_process = MockServerProcess(
+ crashed=True,
+ lines=[
+ 'ActualHash: foobar',
+ 'Content-Type: my_type',
+ 'Content-Transfer-Encoding: none',
+ '#EOF',
+ ])
+
+ content_block = driver._read_block(0, "")
+ self.assertEqual(content_block.content_type, None)
+ self.assertEqual(content_block.encoding, None)
+ self.assertEqual(content_block.content_hash, None)
+ driver._server_process = None
+
def test_read_binary_block(self):
port = TestWebKitPort()
driver = Driver(port, 0, pixel_tests=True)
Modified: trunk/Tools/Scripts/webkitpy/port/server_process.py (218054 => 218055)
--- trunk/Tools/Scripts/webkitpy/port/server_process.py 2017-06-10 19:20:47 UTC (rev 218054)
+++ trunk/Tools/Scripts/webkitpy/port/server_process.py 2017-06-10 19:51:13 UTC (rev 218055)
@@ -316,7 +316,8 @@
# It might be cleaner to pass in the file descriptor to poll instead.
def _read(self, deadline, fetch_bytes_from_buffers_callback):
while True:
- if self.has_crashed():
+ # Polling does not need to occur before bytes are fetched from the buffer.
+ if self._crashed:
return None
if time.time() > deadline:
@@ -327,6 +328,9 @@
if bytes is not None:
return bytes
+ if self.has_crashed():
+ return None
+
if self._use_win32_apis:
self._wait_for_data_and_update_buffers_using_win32_apis(deadline)
else:
Modified: trunk/Tools/Scripts/webkitpy/port/server_process_mock.py (218054 => 218055)
--- trunk/Tools/Scripts/webkitpy/port/server_process_mock.py 2017-06-10 19:20:47 UTC (rev 218054)
+++ trunk/Tools/Scripts/webkitpy/port/server_process_mock.py 2017-06-10 19:51:13 UTC (rev 218055)
@@ -37,17 +37,26 @@
self.env = env
self.started = False
self.stopped = False
+ self.number_of_times_polled = 0
+ def poll(self):
+ self.number_of_times_polled += 1
+
def write(self, bytes):
self.writes.append(bytes)
def has_crashed(self):
+ self.poll()
return self.crashed
def read_stdout_line(self, deadline):
+ if self.has_crashed():
+ return None
return self.lines.pop(0) + "\n"
def read_stdout(self, deadline, size):
+ if self.has_crashed():
+ return None
first_line = self.lines[0]
if size > len(first_line):
self.lines.pop(0)
Modified: trunk/Tools/Scripts/webkitpy/port/server_process_unittest.py (218054 => 218055)
--- trunk/Tools/Scripts/webkitpy/port/server_process_unittest.py 2017-06-10 19:20:47 UTC (rev 218054)
+++ trunk/Tools/Scripts/webkitpy/port/server_process_unittest.py 2017-06-10 19:51:13 UTC (rev 218055)
@@ -99,7 +99,7 @@
class TestServerProcess(unittest.TestCase):
def test_basic(self):
- cmd = [sys.executable, '-c', 'import sys; import time; time.sleep(0.02); print "stdout"; sys.stdout.flush(); print >>sys.stderr, "stderr"']
+ cmd = [sys.executable, '-c', 'import sys; print "stdout"; sys.stdout.flush(); print >>sys.stderr, "stderr"; sys.stdin.readline();']
host = SystemHost()
factory = PortFactory(host)
port = factory.get()
@@ -118,18 +118,62 @@
line = proc.read_stdout_line(now - 1)
self.assertEqual(line, None)
- # FIXME: This part appears to be flaky. line should always be non-None.
- # FIXME: https://bugs.webkit.org/show_bug.cgi?id=88280
line = proc.read_stdout_line(now + 1.0)
- if line:
- self.assertEqual(line.strip(), "stdout")
+ self.assertEqual(line.strip(), "stdout")
line = proc.read_stderr_line(now + 1.0)
- if line:
- self.assertEqual(line.strip(), "stderr")
+ self.assertEqual(line.strip(), "stderr")
+ proc.write('End\n')
proc.stop(0)
+ def test_process_crashing(self):
+ cmd = [sys.executable, '-c', 'import sys; print "stdout 1"; print "stdout 2"; print "stdout 3"; sys.stdout.flush(); sys.stdin.readline(); sys.exit(1);']
+ host = SystemHost()
+ factory = PortFactory(host)
+ port = factory.get()
+ now = time.time()
+ proc = server_process.ServerProcess(port, 'python', cmd)
+ proc.write('')
+
+ line = proc.read_stdout_line(now + 1.0)
+ self.assertEqual(line.strip(), 'stdout 1')
+
+ proc.write('End\n')
+ time.sleep(0.1) # Give process a moment to close.
+
+ line = proc.read_stdout_line(now + 1.0)
+ self.assertEqual(line.strip(), 'stdout 2')
+
+ self.assertEqual(True, proc.has_crashed())
+
+ line = proc.read_stdout_line(now + 1.0)
+ self.assertEqual(line, None)
+
+ proc.stop(0)
+
+ def test_process_crashing_no_data(self):
+ cmd = [sys.executable, '-c',
+ 'import sys; sys.stdin.readline(); sys.exit(1);']
+ host = SystemHost()
+ factory = PortFactory(host)
+ port = factory.get()
+ now = time.time()
+ proc = server_process.ServerProcess(port, 'python', cmd)
+ proc.write('')
+
+ self.assertEqual(False, proc.has_crashed())
+
+ proc.write('End\n')
+ time.sleep(0.1) # Give process a moment to close.
+
+ line = proc.read_stdout_line(now + 1.0)
+ self.assertEqual(line, None)
+
+ self.assertEqual(True, proc.has_crashed())
+
+ proc.stop(0)
+
def test_cleanup(self):
port_obj = TrivialMockPort()
server_process = FakeServerProcess(port_obj=port_obj, name="test", cmd=["test"])
