Bug 145584 - use multiprocessing in run-benchmark to launch web server
Summary: use multiprocessing in run-benchmark to launch web server
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2015-06-02 17:55 PDT by Stephanie Lewis
Modified: 2015-06-05 02:56 PDT (History)
6 users (show)

See Also:

patch (11.44 KB, patch)
2015-06-02 17:55 PDT, Stephanie Lewis
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephanie Lewis 2015-06-02 17:55:57 PDT
Created attachment 254129 [details]

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.
Comment 1 WebKit Commit Bot 2015-06-02 17:59:05 PDT
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.
Comment 2 Ryosuke Niwa 2015-06-02 19:28:10 PDT
Comment on attachment 254129 [details]

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?
Comment 3 Alexey Proskuryakov 2015-06-02 21:46:04 PDT
Comment on attachment 254129 [details]

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.
Comment 4 Stephanie Lewis 2015-06-02 21:55:32 PDT
I don't even have words.

I can move it onto a thread.
Comment 5 Stephanie Lewis 2015-06-05 02:56:00 PDT
Ended up undoing the multiprocessing changes and just keeping the extra error checks to make it more robust.