Telemetry: devtools_http should reuse its http connection.

Telemetry: devtools_http should reuse its http connection.

Previously, devtools_http created a new tcp connection to Chrome for every
request. The large_profile generator makes tens of thousands of requests to
Chrome over a very short period of time. A socket used for a TCP connection
stays in the TIME_WAIT state for 240s after the connection has been closed.
This causes the large_profile generator to exhaust the socket limit on Windows.

Reusing the http connection fixes this problem.

BUG=464609
Committed: https://crrev.com/ea7516d8f1987d4ece3e712e5054a71b906b5603
Cr-Commit-Position: refs/heads/master@{#319849}

[erikchen, 2015-03-07 00:15:19]

erikchen@chromium.org changed reviewers:
+ dtu@chromium.org

[erikchen, 2015-03-07 00:15:19]

dtu: Please review.

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(right):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:25:
the instance is garbage collected.
This class is a member of devtools_client_backend.py, which itself gets cleaned
up during GC, and so on up the ownership chain. Very few instances of
devtools_client_backend.py get created, so it's not a big deal.

[dtu, 2015-03-09 20:39:44]

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(left):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:39:
proxy_handler = urllib2.ProxyHandler({})  # Bypass any system proxy.
What happened with the proxy logic?

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(right):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:42:
raise DevToolsClientConnectionError(e)
I think the way to re-raise and avoid hiding the original stack trace is
something like:
  raise DevToolsClientConnectionError, e, sys.exc_info()[2]

[erikchen, 2015-03-09 23:18:34]

dtu: PTAL

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(left):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:39:
proxy_handler = urllib2.ProxyHandler({})  # Bypass any system proxy.
On 2015/03/09 20:39:44, dtu wrote:
> What happened with the proxy logic?

The library urllib2 uses the default system proxy by default. Skipping the proxy
requires special logic, which you see here. See
  https://docs.python.org/2/library/urllib2.html
for more information.

As far as I can tell, the library httplib doesn't use proxies by default.
Getting it to use a proxy requires invoking the method set_tunnel(). Let me know
if you think I'm interpreting the documentation for httplib incorrectly.

I went ahead and also removed all the proxy logic from the unit test.

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(right):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:42:
raise DevToolsClientConnectionError(e)
On 2015/03/09 20:39:44, dtu wrote:
> I think the way to re-raise and avoid hiding the original stack trace is
> something like:
>   raise DevToolsClientConnectionError, e, sys.exc_info()[2]

You're right. Thanks! I also updated the other "raise" statements in this class.

I made a slight modification - for the second parameter, I passed the tuple (e,)
which passes e as a parameter, to the initializer for
DevToolsClientConnectionError.

[dtu, 2015-03-10 02:06:09]

lgtm with one comment

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(left):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:39:
proxy_handler = urllib2.ProxyHandler({})  # Bypass any system proxy.
On 2015/03/09 23:18:34, erikchen wrote:
> On 2015/03/09 20:39:44, dtu wrote:
> > What happened with the proxy logic?
> 
> The library urllib2 uses the default system proxy by default. Skipping the
proxy
> requires special logic, which you see here. See
>   https://docs.python.org/2/library/urllib2.html
> for more information.
> 
> As far as I can tell, the library httplib doesn't use proxies by default.
> Getting it to use a proxy requires invoking the method set_tunnel(). Let me
know
> if you think I'm interpreting the documentation for httplib incorrectly.
> 
> I went ahead and also removed all the proxy logic from the unit test.

I think that's correct. Maybe add a comment in case anybody wants to change it
again later.

[erikchen, 2015-03-10 04:57:36]

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py
(left):

https://codereview.chromium.org/980073006/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_http.py:39:
proxy_handler = urllib2.ProxyHandler({})  # Bypass any system proxy.
On 2015/03/10 02:06:09, dtu wrote:
> On 2015/03/09 23:18:34, erikchen wrote:
> > On 2015/03/09 20:39:44, dtu wrote:
> > > What happened with the proxy logic?
> > 
> > The library urllib2 uses the default system proxy by default. Skipping the
> proxy
> > requires special logic, which you see here. See
> >   https://docs.python.org/2/library/urllib2.html
> > for more information.
> > 
> > As far as I can tell, the library httplib doesn't use proxies by default.
> > Getting it to use a proxy requires invoking the method set_tunnel(). Let me
> know
> > if you think I'm interpreting the documentation for httplib incorrectly.
> > 
> > I went ahead and also removed all the proxy logic from the unit test.
> 
> I think that's correct. Maybe add a comment in case anybody wants to change it
> again later.

Done.

[erikchen, 2015-03-10 04:57:39]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-03-10 04:57:39]

The patchset sent to the CQ was uploaded after l-g-t-m from dtu@chromium.org
Link to the patchset: https://codereview.chromium.org/980073006/#ps100001
(title: "Comments from dtu, round two.")

[commit-bot: I haz the power, 2015-03-10 04:57:51]

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

[commit-bot: I haz the power, 2015-03-10 07:38:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-10 07:38:45]

Patchset 6 (id:??) landed as
https://crrev.com/ea7516d8f1987d4ece3e712e5054a71b906b5603
Cr-Commit-Position: refs/heads/master@{#319849}