|
|
DescriptionIncrease WebSocket limit to 255
As the existing comments in code indicate the current limit of 30 WebSockets
was chosen arbitrarily. The limit of 30 WebSockets is impacting
real world web sites, for an example, See http://crbug.com/486800
As a reference Firefox has a 200 WebSocket limit.
R=asanka@chromium.org
BUG=486800
Committed: https://crrev.com/64cb80d3dab248fd15972700a71dc2fb1130a809
Cr-Commit-Position: refs/heads/master@{#359051}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated patch to address comments #Patch Set 3 : New patch that clamps max sockets to proxy limits if needed #
Total comments: 1
Patch Set 4 : Updated limit to 255 as requested #
Total comments: 1
Messages
Total messages: 53 (13 generated)
Please review one line change.
asanka@chromium.org changed reviewers: + mmenke@chromium.org - asanka@chromium.org
asanka@chromium.org changed reviewers: + asanka@chromium.org
-> mmenke
asanka@chromium.org changed reviewers: - asanka@chromium.org
mmenke@chromium.org changed reviewers: + ricea@chromium.org
[+ricea]
https://codereview.chromium.org/1130073004/diff/1/net/socket/client_socket_po... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/1130073004/diff/1/net/socket/client_socket_po... net/socket/client_socket_pool_manager.cc:42: // for such use, thus we use a larger limit (of 30) which was determined This CL makes the text you added here incorrect. https://codereview.chromium.org/1130073004/diff/1/net/socket/client_socket_po... net/socket/client_socket_pool_manager.cc:49: // as rlabs.gurulabs.com. See http://crbug.com/486800 There's generally no need to contain a revision log in the file, unless there's some sort of unusual cause for concern or something particularly important to note. git keeps a history if people are curious.
Patchset #2 (id:20001) has been deleted
Sorry for the delayed response; I am on holiday. The short answer for why we can't do this is that it causes debug builds to crash at client_socket_pool_base.cc:177 ie. https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/client_... The longer answer is that there is a subtle interaction between the number of sockets per group and the number of sockets per proxy which requires that inequality at line 177 to be true. I have forgotten what exactly the interaction is; when I get back to work on Friday I will re-read the socket pool code and work it out. However, it is most likely that while "30" was indeed an arbitrary choice, it needs to be less than or equal to 32 for the socket pools to work properly.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
I think it can be made to work if you also change client_socket_pool_manager_impl.cc to respect the proxy limits. Effectively this makes the limit 200 when the connections are direct, and 32 when they go via a proxy. See https://crrev.com/1143183004/ for an example of how to do it.
lgtm mmenke, what do you think?
https://codereview.chromium.org/1130073004/diff/60001/net/socket/client_socke... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/1130073004/diff/60001/net/socket/client_socke... net/socket/client_socket_pool_manager.cc:45: 200 // WEBSOCKET_SOCKET_POOL Can you change this to 255? Then the limit for wss will be the same as the limit for ws.
ricea@chromium.org changed reviewers: + mmenke@chromium.org
Actually adding mmenke for real this time.
Just for the record, I confirmed Dax has indeed signed the CLA. I'm concerned about this change. When using a proxy, one page can easily saturate the websocket pool, so no other page can use websockets. It's basically impossible for the user to figure out what's going wrong when this happens. Even when not behind a proxy, this still makes it a lot easier for one site to fill the pool. Does the websocket spec contain any guidelines for connection limits, beyond throttling rate of establishing new connections?
On 2015/09/01 15:22:13, mmenke wrote: > > Does the websocket spec contain any guidelines for connection limits, beyond > throttling rate of establishing new connections? As a datapoint, Firefox has been using a 200 WebSocket limit (clamped to 32 when using a proxy) since 2011. https://hg.mozilla.org/mozilla-central/rev/43f88c89f4cb
On 2015/09/01 16:25:12, dkelson wrote: > On 2015/09/01 15:22:13, mmenke wrote: > > > > Does the websocket spec contain any guidelines for connection limits, beyond > > throttling rate of establishing new connections? > > As a datapoint, Firefox has been using a 200 WebSocket limit (clamped to 32 when > using a proxy) since 2011. > > https://hg.mozilla.org/mozilla-central/rev/43f88c89f4cb Does Firefox also use a global limit of 32 when using a proxy?
On 2015/09/01 16:35:01, mmenke wrote: > > Does Firefox also use a global limit of 32 when using a proxy? I thought it did, but I'm not sure now. I just looked through Firefox's netwerk/protocol/websocket/nsWebSocketHandler.cpp and I didn't see any signs of Firefox restricting the number of websockets when using a proxy.
On 2015/09/01 15:22:13, mmenke wrote: > I'm concerned about this change. When using a proxy, one page can easily > saturate the websocket pool, so no other page can use websockets. It's > basically impossible for the user to figure out what's going wrong when this > happens. Even when not behind a proxy, this still makes it a lot easier for one > site to fill the pool. Yes, the interaction of WebSockets with proxies is underspecified. As you say, one page can easily exhaust the limit and then no other page can connect. However, this CL doesn't make the situation any worse: it was already possible to hit the limit using unencrypted WebSockets, this CL just makes it possible to hit it with encyrypted WebSockets as well. > Does the websocket spec contain any guidelines for connection limits, beyond > throttling rate of establishing new connections? RFC6455 has this to say: NOTE: There is no limit to the number of established WebSocket connections a client can have with a single remote host. Servers can refuse to accept connections from hosts/IP addresses with an excessive number of existing connections or disconnect resource- hogging connections when suffering high load. In practice, there are always operating system limits, even in the absence of proxies. The browser ought to prevent tabs from interfering with other tabs, but it is hard to balance this against the WebSocket specification's goal of providing TCP/IP-socket-like facilities to web pages. Inter-tab DoS attacks seem to be rare in practice, and similar attacks are possible even with plain HTTP. I don't have a better idea other than to live with it.
On 2015/09/02 06:41:23, Adam Rice wrote: > However, this CL doesn't make the situation any worse: it was already possible > to hit the limit using unencrypted WebSockets, this CL just makes it possible to > hit it with encyrypted WebSockets as well. And we aren't breaking any new ground in behavior. Firefox already has supported 200 WebSocket connections for a long time. This CL clamps the number of WebSocket connections when using a proxy, which Firefox may not do -- so again, not breaking new ground in behavior.
On 2015/09/02 06:41:23, Adam Rice wrote: > On 2015/09/01 15:22:13, mmenke wrote: > > I'm concerned about this change. When using a proxy, one page can easily > > saturate the websocket pool, so no other page can use websockets. It's > > basically impossible for the user to figure out what's going wrong when this > > happens. Even when not behind a proxy, this still makes it a lot easier for > one > > site to fill the pool. > > Yes, the interaction of WebSockets with proxies is underspecified. As you say, > one > page can easily exhaust the limit and then no other page can connect. > > However, this CL doesn't make the situation any worse: it was already possible > to > hit the limit using unencrypted WebSockets, this CL just makes it possible to > hit > it with encyrypted WebSockets as well. > > > Does the websocket spec contain any guidelines for connection limits, beyond > > throttling rate of establishing new connections? > > RFC6455 has this to say: > > NOTE: There is no limit to the number of established WebSocket > connections a client can have with a single remote host. Servers > can refuse to accept connections from hosts/IP addresses with an > excessive number of existing connections or disconnect resource- > hogging connections when suffering high load. > > In practice, there are always operating system limits, even in the absence > of proxies. The browser ought to prevent tabs from interfering with other > tabs, but it is hard to balance this against the WebSocket specification's > goal of providing TCP/IP-socket-like facilities to web pages. > > Inter-tab DoS attacks seem to be rare in practice, and similar attacks are > possible even with plain HTTP. I don't have a better idea other than to live > with it. I'm not concerned about deliberate DoS attacks, I'm concerned about legitimate uses interfering with another.
On 2015/09/02 16:35:46, dkelson wrote: > On 2015/09/02 06:41:23, Adam Rice wrote: > > However, this CL doesn't make the situation any worse: it was already possible > > to hit the limit using unencrypted WebSockets, this CL just makes it possible > to > > hit it with encyrypted WebSockets as well. > > And we aren't breaking any new ground in behavior. Firefox already has supported > 200 WebSocket connections for a long time. This CL clamps the number of > WebSocket connections when using a proxy, which Firefox may not do -- so again, > not breaking new ground in behavior. With the 200 limit, does it have a 256 limit like we do? I'm concerned about the two limits interacting (In ways they don't in FireFox), which is potentially breaking new ground...Or more accurately, potentially breaking new webpages, at least when used in conjunction with each other.
On 2015/09/02 16:47:35, mmenke wrote: > On 2015/09/02 16:35:46, dkelson wrote: > > On 2015/09/02 06:41:23, Adam Rice wrote: > > > However, this CL doesn't make the situation any worse: it was already > possible > > > to hit the limit using unencrypted WebSockets, this CL just makes it > possible > > to > > > hit it with encyrypted WebSockets as well. > > > > And we aren't breaking any new ground in behavior. Firefox already has > supported > > 200 WebSocket connections for a long time. This CL clamps the number of > > WebSocket connections when using a proxy, which Firefox may not do -- so > again, > > not breaking new ground in behavior. > > With the 200 limit, does it have a 256 limit like we do? I'm concerned about > the two limits interacting (In ways they don't in FireFox), which is potentially > breaking new ground...Or more accurately, potentially breaking new webpages, at > least when used in conjunction with each other. I don't want to sound like there's no way I'm going to approve this change, I'd just like a better idea of what we're potentially breaking.
On 2015/09/02 at 16:48:18, mmenke wrote: > On 2015/09/02 16:47:35, mmenke wrote: > > On 2015/09/02 16:35:46, dkelson wrote: > > > On 2015/09/02 06:41:23, Adam Rice wrote: > > > > However, this CL doesn't make the situation any worse: it was already > > possible > > > > to hit the limit using unencrypted WebSockets, this CL just makes it > > possible > > > to > > > > hit it with encyrypted WebSockets as well. > > > > > > And we aren't breaking any new ground in behavior. Firefox already has > > supported > > > 200 WebSocket connections for a long time. This CL clamps the number of > > > WebSocket connections when using a proxy, which Firefox may not do -- so > > again, > > > not breaking new ground in behavior. > > > > With the 200 limit, does it have a 256 limit like we do? I'm concerned about > > the two limits interacting (In ways they don't in FireFox), which is potentially > > breaking new ground...Or more accurately, potentially breaking new webpages, at > > least when used in conjunction with each other. > > I don't want to sound like there's no way I'm going to approve this change, I'd just like a better idea of what we're potentially breaking. Sorry I didn't respond sooner, I wanted to check that my history was correct. From when Chrome first shipped WebSockets until we switched to the new implementation, there was no overall limit except for what was set by the destination server and the client operating system. This was good in terms of spec conformance, but problematic in terms of protecting users and proxies from bad and broken web pages. Firefox shipped WebSockets with a limit of 200. I haven't dug through their history to see whether they always had the limit and whether anyone complained about it. Firefox's limit actually works by throwing a synchronous Javascript exception in the WebSocket destructor, which is probably preferable to many people than our implementation of waiting asynchronously until we either can allocate a socket or timeout. When we switched to the new WebSocket implementation in M38, we added a limit of 256 for ws: and 30 for wss:. Actually, the limit of 30 had been set in 2012, two years before we shipped. Although this was accidental, it narrowly avoided the hard limit of 32 imposed for proxy servers. Since we've only had the limit of 30 on wss: for a year, I think we will not break anything by increasing it. Probably 256 is a better choice than 200, to avoid weird interactions with the overall limit of 256.
On 2015/10/09 09:36:30, Adam Rice wrote: > On 2015/09/02 at 16:48:18, mmenke wrote: > > On 2015/09/02 16:47:35, mmenke wrote: > > > On 2015/09/02 16:35:46, dkelson wrote: > > > > On 2015/09/02 06:41:23, Adam Rice wrote: > > > > > However, this CL doesn't make the situation any worse: it was already > > > possible > > > > > to hit the limit using unencrypted WebSockets, this CL just makes it > > > possible > > > > to > > > > > hit it with encyrypted WebSockets as well. > > > > > > > > And we aren't breaking any new ground in behavior. Firefox already has > > > supported > > > > 200 WebSocket connections for a long time. This CL clamps the number of > > > > WebSocket connections when using a proxy, which Firefox may not do -- so > > > again, > > > > not breaking new ground in behavior. > > > > > > With the 200 limit, does it have a 256 limit like we do? I'm concerned > about > > > the two limits interacting (In ways they don't in FireFox), which is > potentially > > > breaking new ground...Or more accurately, potentially breaking new webpages, > at > > > least when used in conjunction with each other. > > > > I don't want to sound like there's no way I'm going to approve this change, > I'd just like a better idea of what we're potentially breaking. > > Sorry I didn't respond sooner, I wanted to check that my history was correct. > > From when Chrome first shipped WebSockets until we switched to the new > implementation, there was no overall limit except for what was set by the > destination server and the client operating system. This was good in > terms of spec conformance, but problematic in terms of protecting > users and proxies from bad and broken web pages. > > Firefox shipped WebSockets with a limit of 200. I haven't dug through > their history to see whether they always had the limit and whether anyone > complained about it. Firefox's limit actually works by throwing a > synchronous Javascript exception in the WebSocket destructor, which is > probably preferable to many people than our implementation of waiting > asynchronously until we either can allocate a socket or timeout. > > When we switched to the new WebSocket implementation in M38, we added a > limit of 256 for ws: and 30 for wss:. Actually, the limit of 30 had been > set in 2012, two years before we shipped. Although this was accidental, > it narrowly avoided the hard limit of 32 imposed for proxy servers. > > Since we've only had the limit of 30 on wss: for a year, I think we will > not break anything by increasing it. Probably 256 is a better choice than > 200, to avoid weird interactions with the overall limit of 256. I'm not following - what weird interactions? Seems like having them at the same value is precisely the sort of thing to cause "weird" interactions.
On 2015/10/09 15:18:59, mmenke wrote: > I'm not following - what weird interactions? Seems like having them at the same > value is precisely the sort of thing to cause "weird" interactions. I'm not sure. If you feel that 200 is the safer option, I'm happy to go with that. There are more people complaining on the bug, so I'd like to just do what we can for the time being.
On Wed, Oct 28, 2015 at 9:45 AM, <ricea@chromium.org> wrote: > > I'm not sure. If you feel that 200 is the safer option, I'm happy to go > with > that. There are more people complaining on the bug, so I'd like to just do > what > we can for the time being. > I'd love to get this bug fixed so I can just use Chrome. In our environment we've had to install Firefox for all of our users. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
mmenke@chromium.org changed reviewers: + eroman@chromium.org
[+eroman]: Do you have any opinion on this, as it relates to proxies? In particular, it ups the number of websocket connections to 200, even when behind a proxy, which rather exceeds our current proxy connection limit of 32.
On 2015/10/28 16:22:01, mmenke wrote: > [+eroman]: Do you have any opinion on this, as it relates to proxies? In > particular, it ups the number of websocket connections to 200, even when behind > a proxy, which rather exceeds our current proxy connection limit of 32. The conclusion I came to when I traced through the code 5 months ago was that if and only if we are going through a proxy, the pool size will be clamped to 32, and everything will work. However, I would welcome a second opinion.
On 2015/10/28 17:41:55, Adam Rice wrote: > On 2015/10/28 16:22:01, mmenke wrote: > > [+eroman]: Do you have any opinion on this, as it relates to proxies? In > > particular, it ups the number of websocket connections to 200, even when > behind > > a proxy, which rather exceeds our current proxy connection limit of 32. > > The conclusion I came to when I traced through the code 5 months ago was that > if and only if we are going through a proxy, the pool size will be clamped to > 32, and everything will work. > > However, I would welcome a second opinion. You're right - I was thinking it was std::max, not std::min, for some reason. So with a proxy, the group limit and pool limit are the same, for websockets.
Can you add some tests? Otherwise the code change itself LGTM. This is a large increase in the limit, Adam will you be monitoring for any fallout from this?
https://codereview.chromium.org/1130073004/diff/80001/net/socket/client_socke... File net/socket/client_socket_pool_manager.cc (right): https://codereview.chromium.org/1130073004/diff/80001/net/socket/client_socke... net/socket/client_socket_pool_manager.cc:41: // than normal other connections. Use a limit of 255, so the limit for wss will The CL description says the limit is 200. Update either the code of the description so they match.
On Wed, Oct 28, 2015 at 1:06 PM, <eroman@chromium.org> wrote: > The CL description says the limit is 200. Update either the code of the > description so they match. > They do match. The 200 in the comment is just noting what Firefox uses. Here is comment and code for reference. // WebSocket connections are long-lived, and should be treated differently // than normal other connections. Use a limit of 255, so the limit for wss will // be the same as the limit for ws. Also note that Firefox uses a limit of 200. // See http://crbug.com/486800 int g_max_sockets_per_group[] = { 6, // NORMAL_SOCKET_POOL 255 // WEBSOCKET_SOCKET_POOL }; To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/28 19:05:18, eroman wrote: > Can you add some tests? > > Otherwise the code change itself LGTM. > > This is a large increase in the limit, Adam will you be monitoring for any > fallout from this? I will keep an eye on the histogram Net.WebSocket.ErrorCodes. I can quickly modify WebSocketBrowserTest.SSLConnectionLimit (in //chrome/browser/net/websocket_browsertest.cc) once the CL has landed to confirm it does what it claims. I'm not sure where best to test the proxy case. //net/websockets/websocket_end_to_end_test.cc is one possibility.
The CL descriptions reads: "Increase WebSocket limit to 200 (matches Firefox)" However that is clearly not the same as the constant used in this file. On Wed, Oct 28, 2015 at 12:15 PM, Dax Kelson <dkelson@gurulabs.com> wrote: > On Wed, Oct 28, 2015 at 1:06 PM, <eroman@chromium.org> wrote: > >> The CL description says the limit is 200. Update either the code of the >> description so they match. >> > > They do match. The 200 in the comment is just noting what Firefox uses. > > Here is comment and code for reference. > > // WebSocket connections are long-lived, and should be treated differently > // than normal other connections. Use a limit of 255, so the limit for wss > will > // be the same as the limit for ws. Also note that Firefox uses a limit of > 200. > // See http://crbug.com/486800 > int g_max_sockets_per_group[] = { > 6, // NORMAL_SOCKET_POOL > 255 // WEBSOCKET_SOCKET_POOL > }; > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Increase WebSocket limit to 200 (matches Firefox) As the existing comments in code indicate the current limit of 30 WebSockets was chosen arbitrarily. The limit of 30 WebSockets is impacting real world web sites, for an example, See http://crbug.com/486800 Patch matches Firefox's 200 WebSocket limit. R=asanka@chromium.org BUG=486800 ========== to ========== Increase WebSocket limit to 255 As the existing comments in code indicate the current limit of 30 WebSockets was chosen arbitrarily. The limit of 30 WebSockets is impacting real world web sites, for an example, See http://crbug.com/486800 As a reference Firefox has a 200 WebSocket limit. R=asanka@chromium.org BUG=486800 ==========
On 2015/10/28 19:24:49, eroman wrote: > The CL descriptions reads: Sorry, misunderstood. The CL description has been fixed.
So is everything good to go?
yes, still LGTM
The CQ bit was checked by ricea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130073004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1130073004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
dkelson: I think you can click the Commit checkbox.
The CQ bit was checked by dkelson@gurulabs.com
The patchset sent to the CQ was uploaded after l-g-t-m from ricea@chromium.org Link to the patchset: https://codereview.chromium.org/1130073004/#ps80001 (title: "Updated limit to 255 as requested")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130073004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1130073004/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/64cb80d3dab248fd15972700a71dc2fb1130a809 Cr-Commit-Position: refs/heads/master@{#359051} |