Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1338)

Issue 7289006: Basic HTTP pipelining support (Closed)

Created:
9 years, 5 months ago by James Simonsen
Modified:
9 years, 2 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

Basic HTTP pipelining support. This must be enabled in about:flags. It is naive and assumes all servers correctly implement pipelining. Proxies are not supported. Immediate future work: - Integration tests. - Additional NetLog logging. - Refactor HttpPipelinedConnectionImpl. Long term: - Detect broken transparent proxies. - Detect and/or mitigate broken servers. - Buffer HttpPipelinedStream. - Optimize number of pipelines and their depth. - Support proxies. BUG=8991 TEST=net_unittests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106364

Patch Set 1 #

Patch Set 2 : Use HttpStreamFactoryImpl::Job #

Total comments: 10

Patch Set 3 : Added about:flags entry #

Total comments: 18

Patch Set 4 : Added unit tests #

Total comments: 55

Patch Set 5 : Better Close() handling and tests #

Total comments: 24

Patch Set 6 : Fixed transaction. #

Total comments: 17

Patch Set 7 : Simplify transaction unit tests #

Total comments: 21

Patch Set 8 : Change pool API #

Total comments: 24

Patch Set 9 : Fix races #

Total comments: 13

Patch Set 10 : Use linked_ptr #

Total comments: 3

Patch Set 11 : Move HttpPipelinedHostPool #

Patch Set 12 : Fix trybot issues #

Total comments: 1

Patch Set 13 : Fix eviction #

Patch Set 14 : Merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3022 lines, -109 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -0 lines 0 comments Download
M net/base/net_error_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M net/base/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -0 lines 0 comments Download
M net/http/http_basic_stream.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M net/http/http_basic_stream.cc View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 0 comments Download
M net/http/http_network_transaction.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +9 lines, -10 lines 0 comments Download
M net/http/http_network_transaction_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +59 lines, -4 lines 0 comments Download
A net/http/http_pipelined_connection.h View 1 2 3 4 5 6 7 8 9 1 chunk +74 lines, -0 lines 0 comments Download
A net/http/http_pipelined_connection_impl.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +271 lines, -0 lines 0 comments Download
A net/http/http_pipelined_connection_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +612 lines, -0 lines 0 comments Download
A net/http/http_pipelined_connection_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1067 lines, -0 lines 0 comments Download
A net/http/http_pipelined_host.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +93 lines, -0 lines 0 comments Download
A net/http/http_pipelined_host.cc View 1 2 3 4 5 6 7 8 9 1 chunk +108 lines, -0 lines 0 comments Download
A net/http/http_pipelined_host_pool.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +66 lines, -0 lines 0 comments Download
A net/http/http_pipelined_host_pool.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +80 lines, -0 lines 0 comments Download
A net/http/http_pipelined_host_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +201 lines, -0 lines 0 comments Download
A + net/http/http_pipelined_stream.h View 1 2 3 4 5 6 7 8 2 chunks +33 lines, -29 lines 0 comments Download
A net/http/http_pipelined_stream.cc View 1 2 3 4 5 6 7 8 1 chunk +142 lines, -0 lines 0 comments Download
M net/http/http_response_body_drainer_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M net/http/http_stream.h View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 0 comments Download
M net/http/http_stream_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +7 lines, -0 lines 0 comments Download
M net/http/http_stream_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M net/http/http_stream_factory_impl.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +12 lines, -1 line 0 comments Download
M net/http/http_stream_factory_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +22 lines, -1 line 0 comments Download
M net/http/http_stream_factory_impl_job.h View 1 2 3 4 5 6 7 8 3 chunks +6 lines, -2 lines 0 comments Download
M net/http/http_stream_factory_impl_job.cc View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +48 lines, -57 lines 0 comments Download
M net/http/http_stream_factory_impl_request.h View 1 2 3 4 3 chunks +11 lines, -1 line 0 comments Download
M net/http/http_stream_factory_impl_request.cc View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +32 lines, -3 lines 0 comments Download
M net/http/http_stream_parser.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
M net/net.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +11 lines, -0 lines 0 comments Download
M net/spdy/spdy_http_stream.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M net/spdy/spdy_http_stream.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 45 (0 generated)
willchan no longer on Chromium
Hi! I'm glad to work has started on this. It actually looks remarkably like how ...
9 years, 5 months ago (2011-07-06 07:01:53 UTC) #1
James Simonsen
Hi Will, Thanks for your feedback. I definitely need guidance on this, since I'm new ...
9 years, 5 months ago (2011-07-14 04:29:14 UTC) #2
willchan no longer on Chromium
On 2011/07/14 04:29:14, James Simonsen wrote: > Hi Will, Thanks for your feedback. I definitely ...
9 years, 5 months ago (2011-07-14 16:38:55 UTC) #3
James Simonsen
Thanks for your help, I think I understand everything better. On 2011/07/14 16:38:55, willchan wrote: ...
9 years, 5 months ago (2011-07-15 05:23:10 UTC) #4
willchan no longer on Chromium
On Fri, Jul 15, 2011 at 8:23 AM, <simonjam@chromium.org> wrote: > Thanks for your help, ...
9 years, 5 months ago (2011-07-15 06:03:18 UTC) #5
James Simonsen
Alright, I've updated it to follow the general pattern that SPDY uses. Would you mind ...
9 years, 5 months ago (2011-07-17 01:39:30 UTC) #6
willchan no longer on Chromium
On 2011/07/17 01:39:30, James Simonsen wrote: > Alright, I've updated it to follow the general ...
9 years, 5 months ago (2011-07-18 13:03:21 UTC) #7
James Simonsen
On 2011/07/18 13:03:21, willchan wrote: > On 2011/07/17 01:39:30, James Simonsen wrote: > > Alright, ...
9 years, 5 months ago (2011-07-18 19:25:23 UTC) #8
willchan no longer on Chromium
I've started looking at this, but this will require a much more detailed look. This ...
9 years, 5 months ago (2011-07-19 13:29:38 UTC) #9
James Simonsen
I'll do unit tests next, but I'm a little distracted right now, so it may ...
9 years, 5 months ago (2011-07-19 22:54:23 UTC) #10
mmenke
Chris suggested that I help review this CL, with Will taking lead. A couple preliminary ...
9 years, 4 months ago (2011-08-03 21:09:39 UTC) #11
James Simonsen
Thanks for the additional feedback! I've also added unit tests and fixed any associated bugs. ...
9 years, 4 months ago (2011-08-05 01:39:00 UTC) #12
mmenke
Sorry I'm so late getting back to you. There are huge swaths of the network ...
9 years, 4 months ago (2011-08-23 19:05:25 UTC) #13
mmenke
http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_connection_impl.cc File net/http/http_pipelined_connection_impl.cc (right): http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_connection_impl.cc#newcode211 net/http/http_pipelined_connection_impl.cc:211: if (result < OK) { On ERR_SOCKET_NOT_CONNECTED, we might ...
9 years, 4 months ago (2011-08-23 19:15:25 UTC) #14
mmenke
Found a crasher while playing with the code. http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_connection_impl.cc File net/http/http_pipelined_connection_impl.cc (right): http://codereview.chromium.org/7289006/diff/21001/net/http/http_pipelined_connection_impl.cc#newcode444 net/http/http_pipelined_connection_impl.cc:444: DCHECK(pipeline_id ...
9 years, 4 months ago (2011-08-23 20:35:12 UTC) #15
James Simonsen
Thanks for the detailed review! I think you really dug up a lot of good ...
9 years, 4 months ago (2011-08-26 22:19:07 UTC) #16
willchan no longer on Chromium
Just got back from vacation, I hope to get around to this today. If I ...
9 years, 3 months ago (2011-09-01 21:11:38 UTC) #17
mmenke
A couple more comments. http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_transaction.cc#newcode479 net/http/http_network_transaction.cc:479: RestartIgnoringLastError(user_callback_); You want to return ...
9 years, 3 months ago (2011-09-01 21:15:39 UTC) #18
willchan no longer on Chromium
I've only just started to take a look. I primarily looked at the HttpStreamFactoryImpl integration ...
9 years, 3 months ago (2011-09-03 01:11:06 UTC) #19
James Simonsen
http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/7289006/diff/38001/net/http/http_network_transaction.cc#newcode479 net/http/http_network_transaction.cc:479: RestartIgnoringLastError(user_callback_); On 2011/09/01 21:15:39, Matt Menke wrote: > You ...
9 years, 3 months ago (2011-09-07 21:20:10 UTC) #20
mmenke
A couple more mostly minor comments. http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc#newcode408 chrome/browser/about_flags.cc:408: "enable-http-pipelining", You used ...
9 years, 3 months ago (2011-09-15 19:28:15 UTC) #21
willchan no longer on Chromium
Apologies, code yellow is slowing me down. I'll get to this eventually =/ On Sep ...
9 years, 3 months ago (2011-09-15 19:44:47 UTC) #22
James Simonsen
http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/7289006/diff/55001/chrome/browser/about_flags.cc#newcode408 chrome/browser/about_flags.cc:408: "enable-http-pipelining", On 2011/09/15 19:28:16, Matt Menke wrote: > You ...
9 years, 3 months ago (2011-09-17 01:23:02 UTC) #23
mmenke
http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_connection_impl.cc File net/http/http_pipelined_connection_impl.cc (right): http://codereview.chromium.org/7289006/diff/55001/net/http/http_pipelined_connection_impl.cc#newcode221 net/http/http_pipelined_connection_impl.cc:221: callback->Run(result); Yea, I was just thinking that we don't ...
9 years, 3 months ago (2011-09-17 01:41:22 UTC) #24
willchan no longer on Chromium
Here are some more comments finally! I have mostly read through all the HttpStreamFactoryImpl::Job integration ...
9 years, 2 months ago (2011-09-26 23:49:20 UTC) #25
James Simonsen
I still need to look into the integration test that Matt was talking about, but ...
9 years, 2 months ago (2011-09-29 02:39:31 UTC) #26
willchan no longer on Chromium
http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/7289006/diff/68001/net/http/http_network_transaction.cc#newcode571 net/http/http_network_transaction.cc:571: if (rv == ERR_PIPELINE_EVICTION) { On 2011/09/29 02:39:31, James ...
9 years, 2 months ago (2011-09-29 19:32:48 UTC) #27
willchan no longer on Chromium
HttpPipelinedConnectionImpl is complicated, I am trying to fit everything into my head. Sending more comments. ...
9 years, 2 months ago (2011-09-29 20:20:07 UTC) #28
willchan no longer on Chromium
Another interesting thing I've noticed about this architecture is that it lets a single request ...
9 years, 2 months ago (2011-09-29 23:36:02 UTC) #29
willchan no longer on Chromium
The HttpPipelinedConnectionImpl is hard for me to grok. I'm not sure if there's a better ...
9 years, 2 months ago (2011-09-29 23:38:17 UTC) #30
James Simonsen
Done gardening. Time to get this landed... Agreed that this HttpPipelinedConnectionImpl could use another refactoring. ...
9 years, 2 months ago (2011-10-12 01:36:58 UTC) #31
mmenke
A couple more nits. I defer to Will on deciding what should be done before ...
9 years, 2 months ago (2011-10-12 19:41:35 UTC) #32
James Simonsen
http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_transaction.cc#newcode1186 net/http/http_network_transaction.cc:1186: make_scoped_refptr(new NetLogIntegerParameter("net_error", error))); On 2011/10/12 19:41:35, Matt Menke wrote: ...
9 years, 2 months ago (2011-10-12 21:55:51 UTC) #33
willchan no longer on Chromium
I will take a look later today. James, I am out half of tomorrow and ...
9 years, 2 months ago (2011-10-12 22:05:26 UTC) #34
willchan no longer on Chromium
lgtm http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factory_impl_job.cc File net/http/http_stream_factory_impl_job.cc (right): http://codereview.chromium.org/7289006/diff/68001/net/http/http_stream_factory_impl_job.cc#newcode811 net/http/http_stream_factory_impl_job.cc:811: HttpPipelinedConnection* pipeline = session_->http_pipelined_host_pool()-> On 2011/09/29 02:39:31, James ...
9 years, 2 months ago (2011-10-13 02:29:16 UTC) #35
willchan no longer on Chromium
Please update your changelist description to describe the state of the HTTP pipelining implementation as ...
9 years, 2 months ago (2011-10-13 02:30:59 UTC) #36
mmenke
Going to give this another quick once over tomorrow before signing off. http://codereview.chromium.org/7289006/diff/101002/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc ...
9 years, 2 months ago (2011-10-13 03:02:48 UTC) #37
James Simonsen
I'll address the rest of the comments tomorrow. I just wanted to answer this question ...
9 years, 2 months ago (2011-10-13 03:35:11 UTC) #38
willchan no longer on Chromium
On Wed, Oct 12, 2011 at 8:35 PM, <simonjam@chromium.org> wrote: > I'll address the rest ...
9 years, 2 months ago (2011-10-13 03:36:31 UTC) #39
James Simonsen
http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factory.h File net/http/http_stream_factory.h (right): http://codereview.chromium.org/7289006/diff/79001/net/http/http_stream_factory.h#newcode180 net/http/http_stream_factory.h:180: virtual void OnHttpPipelinedHostHasAdditionalCapacity( On 2011/09/29 23:36:02, willchan wrote: > ...
9 years, 2 months ago (2011-10-13 19:29:57 UTC) #40
mmenke
LGTM. The M16 branch point is late this coming Monday evening/early Tuesday. Mind putting off ...
9 years, 2 months ago (2011-10-14 16:21:26 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/7289006/142001
9 years, 2 months ago (2011-10-19 18:40:29 UTC) #42
commit-bot: I haz the power
Can't apply patch for file chrome/common/chrome_switches.cc. While running patch -p1 --forward --force; patching file chrome/common/chrome_switches.cc ...
9 years, 2 months ago (2011-10-19 18:40:35 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/7289006/146001
9 years, 2 months ago (2011-10-19 19:02:32 UTC) #44
commit-bot: I haz the power
9 years, 2 months ago (2011-10-19 20:14:33 UTC) #45
Change committed as 106364

Powered by Google App Engine
This is Rietveld 408576698