Bug 145584 – use multiprocessing in run-benchmark to launch web server

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Stephanie Lewis]

Created attachment 254129 [details]
patch

Was debugging some hanging issues in run-benchmark and I ended up moving the separate web process to multiprocessing to help me debug.
Clean up some exception handling and style issues.
Wait for the server to be ready to serve pages before returning.

[WebKit Commit Bot]

Attachment 254129 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:71:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:11:  No name 'web' in module 'twisted'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:12:  No name 'web' in module 'twisted'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:13:  No name 'internet' in module 'twisted'  [pylint/E0611] [5]
ERROR: Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 254129 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254129&action=review

> Tools/ChangeLog:4
> +

Missing bug URL.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:35
> +        except Exception as e:

Spell out exception or error.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:65
> +                        command = ['/usr/sbin/lsof', '-a', '-iTCP', '-sTCP:LISTEN', '-p', str(self.server_process.pid)]
> +                        output = subprocess.check_output(command)

Perhaps we don't really need a local variable `command` here?

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:66
> +                        self.server_port = int(re.findall('TCP \*:(\d+) \(LISTEN\)', output)[0])

Why don't we just use re.search if we're grabbing the first result?

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:71
> +                    except Exception as e:
> +                         _log.info('Error: %s' % e)

nit: spell out e.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:76
> +            except Exception as e:
> +                sys.exit("Cannot listen to server, max tries exceeded: %s" % e)

Ditto. Also, why don't we use _log.error and sys.exit separately as done elsewhere for consistency?

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:87
> +        except Exception as e:
> +            _log.info('Server not running, max tries exceeded: %s' % e)

This code never runs unless _log.info or time.sleep blows up.
I think what you wanna do is to return immediately after subprocess.check_call.
And just error and sys.exit when we got out of the for loop without hitting that return.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:121
> +    def postResult(self, result):

Can we rename this to post_result for consistency if we're using PEP style?

[Alexey Proskuryakov]

Comment on attachment 254129 [details]
patch

Please do not use multiprocessing. It is broken by design, and we were told that we simply should not use it (see e.g. <rdar://problem/19851334>).

The recommendation is to use either threading or subprocess.

[Stephanie Lewis]

I don't even have words.

I can move it onto a thread.

[Stephanie Lewis]

Ended up undoing the multiprocessing changes and just keeping the extra error checks to make it more robust.

http://trac.webkit.org/changeset/185244