Introduce a policy to control the maximal number of connections per proxy server.

Introduce a policy to control the maximal number of connections per proxy server.

BUG=63658
TEST=All policy unit_tests should pass.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91842

[pastarmovj, 2011-06-29 21:25:25]

@gfeher: Please review the policy definition part.
@willchan: Please review the net part of the CL.
Thank you both!

[willchan no longer on Chromium, 2011-06-29 22:54:55]

http://codereview.chromium.org/7283018/diff/1/chrome/browser/net/net_pref_obs...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/7283018/diff/1/chrome/browser/net/net_pref_obs...
chrome/browser/net/net_pref_observer.cc:74: if (!pref_name || *pref_name ==
prefs::kMaxConnectionsPerProxy) {
There are a few issues here. First, this is only safe to run on the UI thread
before the IO thread has started. Otherwise, it needs to run on the IO thread.
Also, it doesn't work dynamically. The global is only read when creating the
socket pools, which only happens when a profile is created. Is that ok?

http://codereview.chromium.org/7283018/diff/1/chrome/browser/net/net_pref_obs...
chrome/browser/net/net_pref_observer.cc:98: 32,
This should probably use a default that is read from net/. Perhaps you should
take the value of 32 from client_socket_pool_manager.cc and make it a global
named constant, so it can be used here.

http://codereview.chromium.org/7283018/diff/1/net/socket/client_socket_pool_m...
File net/socket/client_socket_pool_manager.cc (right):

http://codereview.chromium.org/7283018/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.cc:570: LOG(ERROR) << "### Set the count
to : " << socket_count;
Remove.

[gfeher, 2011-06-30 11:07:57]

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:1580: The value of this policy should be
lower than 100 and higher than 6 and the default value is 32.''',
Are 100 and 6 are suggested or enforced values? What happens if the specified
value is out of the range? My suggestion: clamping.

[pastarmovj, 2011-07-01 18:52:43]

Moved the policy to a static init at browser start. Had to make the code disable
field trials if policy is set because I was initializing it even earlier than
they did :)

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:1580: The value of this policy should be
lower than 100 and higher than 6 and the default value is 32.''',
On 2011/06/30 11:07:57, gfeher wrote:
> Are 100 and 6 are suggested or enforced values? What happens if the specified
> value is out of the range? My suggestion: clamping.

These are enforced values and are checked in the code too. But I think Will will
restate this documentation anyhow.

http://codereview.chromium.org/7283018/diff/1/chrome/browser/net/net_pref_obs...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/7283018/diff/1/chrome/browser/net/net_pref_obs...
chrome/browser/net/net_pref_observer.cc:74: if (!pref_name || *pref_name ==
prefs::kMaxConnectionsPerProxy) {
On 2011/06/29 22:54:55, willchan wrote:
> There are a few issues here. First, this is only safe to run on the UI thread
> before the IO thread has started. Otherwise, it needs to run on the IO thread.
> Also, it doesn't work dynamically. The global is only read when creating the
> socket pools, which only happens when a profile is created. Is that ok?

Ok i removed the dynamic marking from the policy and the observer for it. So it
will only be set once on browser boot.

http://codereview.chromium.org/7283018/diff/1/chrome/browser/net/net_pref_obs...
chrome/browser/net/net_pref_observer.cc:98: 32,
On 2011/06/29 22:54:55, willchan wrote:
> This should probably use a default that is read from net/. Perhaps you should
> take the value of 32 from client_socket_pool_manager.cc and make it a global
> named constant, so it can be used here.

Done.

http://codereview.chromium.org/7283018/diff/1/net/socket/client_socket_pool_m...
File net/socket/client_socket_pool_manager.cc (right):

http://codereview.chromium.org/7283018/diff/1/net/socket/client_socket_pool_m...
net/socket/client_socket_pool_manager.cc:570: LOG(ERROR) << "### Set the count
to : " << socket_count;
On 2011/06/29 22:54:55, willchan wrote:
> Remove.

Done.

[pastarmovj, 2011-07-07 16:49:22]

Ping?

[willchan no longer on Chromium, 2011-07-07 17:22:04]

LGTM

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:1580: The value of this policy should be
lower than 100 and higher than 6 and the default value is 32.''',
On 2011/07/01 18:52:44, pastarmovj wrote:
> On 2011/06/30 11:07:57, gfeher wrote:
> > Are 100 and 6 are suggested or enforced values? What happens if the
specified
> > value is out of the range? My suggestion: clamping.
> 
> These are enforced values and are checked in the code too. But I think Will
will
> restate this documentation anyhow.

This documentation is actually pretty good. The one thing to mention is that
some web apps are known to consume many connections with hanging GETs, so
lowering below 32 may lead to browser networking hangs if too many such web apps
are open. Lower below the default at your own risk.

http://codereview.chromium.org/7283018/diff/5001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_manager.cc (right):

http://codereview.chromium.org/7283018/diff/5001/net/socket/client_socket_poo...
net/socket/client_socket_pool_manager.cc:572: LOG(ERROR) << "### Set the count
to : " << socket_count;
I presume this is for debugging. If so, please kill it.

[pastarmovj, 2011-07-08 10:49:39]

Cleaned up the suggestions as well as some leftovers from the first version.
Assuming this won't invalidate your LGTM so I will click the commit checkbox.

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:1580: The value of this policy should be
lower than 100 and higher than 6 and the default value is 32.''',
On 2011/07/07 17:22:04, willchan wrote:
> On 2011/07/01 18:52:44, pastarmovj wrote:
> > On 2011/06/30 11:07:57, gfeher wrote:
> > > Are 100 and 6 are suggested or enforced values? What happens if the
> specified
> > > value is out of the range? My suggestion: clamping.
> > 
> > These are enforced values and are checked in the code too. But I think Will
> will
> > restate this documentation anyhow.
> 
> This documentation is actually pretty good. The one thing to mention is that
> some web apps are known to consume many connections with hanging GETs, so
> lowering below 32 may lead to browser networking hangs if too many such web
apps
> are open. Lower below the default at your own risk.

Done.

http://codereview.chromium.org/7283018/diff/5001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/7283018/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:899: std::max(std::min(max_per_proxy,
100),
Made the max 99 because the docu says lower than 100 and the DCHECKS in the
setter actually compare for strict inequality.

http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
File chrome/browser/net/net_pref_observer.cc (right):

http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
chrome/browser/net/net_pref_observer.cc:86: PrefService::UNSYNCABLE_PREF);
Put back the new line here (reduce turbulence in code).

http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
File chrome/browser/net/net_pref_observer.h (right):

http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
chrome/browser/net/net_pref_observer.h:47: IntegerPrefMember
max_connections_per_proxy_;
Cleaned up that.

http://codereview.chromium.org/7283018/diff/5001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_manager.cc (right):

http://codereview.chromium.org/7283018/diff/5001/net/socket/client_socket_poo...
net/socket/client_socket_pool_manager.cc:572: LOG(ERROR) << "### Set the count
to : " << socket_count;
On 2011/07/07 17:22:04, willchan wrote:
> I presume this is for debugging. If so, please kill it.

Done.

[willchan no longer on Chromium, 2011-07-08 10:54:57]

Sounds good.

On Fri, Jul 8, 2011 at 1:49 PM,  <pastarmovj@chromium.org> wrote:
> Cleaned up the suggestions as well as some leftovers from the first version.
> Assuming this won't invalidate your LGTM so I will click the commit
> checkbox.
>
>
>
http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
> File chrome/app/policy/policy_templates.json (right):
>
>
http://codereview.chromium.org/7283018/diff/1/chrome/app/policy/policy_templa...
> chrome/app/policy/policy_templates.json:1580: The value of this policy
> should be lower than 100 and higher than 6 and the default value is
> 32.''',
> On 2011/07/07 17:22:04, willchan wrote:
>>
>> On 2011/07/01 18:52:44, pastarmovj wrote:
>> > On 2011/06/30 11:07:57, gfeher wrote:
>> > > Are 100 and 6 are suggested or enforced values? What happens if
>
> the
>>
>> specified
>> > > value is out of the range? My suggestion: clamping.
>> >
>> > These are enforced values and are checked in the code too. But I
>
> think Will
>>
>> will
>> > restate this documentation anyhow.
>
>> This documentation is actually pretty good. The one thing to mention
>
> is that
>>
>> some web apps are known to consume many connections with hanging GETs,
>
> so
>>
>> lowering below 32 may lead to browser networking hangs if too many
>
> such web apps
>>
>> are open. Lower below the default at your own risk.
>
> Done.
>
>
http://codereview.chromium.org/7283018/diff/5001/chrome/browser/browser_proce...
> File chrome/browser/browser_process_impl.cc (right):
>
>
http://codereview.chromium.org/7283018/diff/5001/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:899:
> std::max(std::min(max_per_proxy, 100),
> Made the max 99 because the docu says lower than 100 and the DCHECKS in
> the setter actually compare for strict inequality.
>
>
http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
> File chrome/browser/net/net_pref_observer.cc (right):
>
>
http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
> chrome/browser/net/net_pref_observer.cc:86:
> PrefService::UNSYNCABLE_PREF);
> Put back the new line here (reduce turbulence in code).
>
>
http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
> File chrome/browser/net/net_pref_observer.h (right):
>
>
http://codereview.chromium.org/7283018/diff/5001/chrome/browser/net/net_pref_...
> chrome/browser/net/net_pref_observer.h:47: IntegerPrefMember
> max_connections_per_proxy_;
> Cleaned up that.
>
>
http://codereview.chromium.org/7283018/diff/5001/net/socket/client_socket_poo...
> File net/socket/client_socket_pool_manager.cc (right):
>
>
http://codereview.chromium.org/7283018/diff/5001/net/socket/client_socket_poo...
> net/socket/client_socket_pool_manager.cc:572: LOG(ERROR) << "### Set the
> count to : " << socket_count;
> On 2011/07/07 17:22:04, willchan wrote:
>>
>> I presume this is for debugging. If so, please kill it.
>
> Done.
>
> http://codereview.chromium.org/7283018/
>

[commit-bot: I haz the power, 2011-07-08 12:17:20]

Change committed as 91842