Chromium Code Reviews
Help | Chromium Project | Sign in
(186)

Issue 7289006: Basic HTTP pipelining support (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by James Simonsen
Modified:
2 years, 6 months ago
CC:
chromium-reviews_chromium.org, 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) Lint 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 ? errors 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 1 errors 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 ? errors 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 0 errors 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 0 errors 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 1 errors 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 ? errors 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 0 errors 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 0 errors 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 0 errors 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 ? errors 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 2 errors 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 1 errors 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 1 errors 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 3 errors 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 2 errors 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 2 errors 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 1 errors 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 1 errors 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 1 errors 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 1 errors 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 2 errors 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 1 errors Download
M net/http/http_stream.h View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 0 comments 0 errors 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 0 errors 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 0 errors 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 0 errors 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 ? errors 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 0 errors 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 ? errors Download
M net/http/http_stream_factory_impl_request.h View 1 2 3 4 3 chunks +11 lines, -1 line 0 comments 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 45
willchan
Hi! I'm glad to work has started on this. It actually looks remarkably like how ...
2 years, 9 months ago #1
James Simonsen
Hi Will, Thanks for your feedback. I definitely need guidance on this, since I'm new ...
2 years, 9 months ago #2
willchan
On 2011/07/14 04:29:14, James Simonsen wrote: > Hi Will, Thanks for your feedback. I definitely ...
2 years, 9 months ago #3
James Simonsen
Thanks for your help, I think I understand everything better. On 2011/07/14 16:38:55, willchan wrote: ...
2 years, 9 months ago #4
willchan
On Fri, Jul 15, 2011 at 8:23 AM, <simonjam@chromium.org> wrote: > Thanks for your help, ...
2 years, 9 months ago #5
James Simonsen
Alright, I've updated it to follow the general pattern that SPDY uses. Would you mind ...
2 years, 9 months ago #6
willchan
On 2011/07/17 01:39:30, James Simonsen wrote: > Alright, I've updated it to follow the general ...
2 years, 9 months ago #7
James Simonsen
On 2011/07/18 13:03:21, willchan wrote: > On 2011/07/17 01:39:30, James Simonsen wrote: > > Alright, ...
2 years, 9 months ago #8
willchan
I've started looking at this, but this will require a much more detailed look. This ...
2 years, 9 months ago #9
James Simonsen
I'll do unit tests next, but I'm a little distracted right now, so it may ...
2 years, 9 months ago #10
mmenke
Chris suggested that I help review this CL, with Will taking lead. A couple preliminary ...
2 years, 8 months ago #11
James Simonsen
Thanks for the additional feedback! I've also added unit tests and fixed any associated bugs. ...
2 years, 8 months ago #12
mmenke
Sorry I'm so late getting back to you. There are huge swaths of the network ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #15
James Simonsen
Thanks for the detailed review! I think you really dug up a lot of good ...
2 years, 7 months ago #16
willchan
Just got back from vacation, I hope to get around to this today. If I ...
2 years, 7 months ago #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 ...
2 years, 7 months ago #18
willchan
I've only just started to take a look. I primarily looked at the HttpStreamFactoryImpl integration ...
2 years, 7 months ago #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 ...
2 years, 7 months ago #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 ...
2 years, 7 months ago #21
willchan
Apologies, code yellow is slowing me down. I'll get to this eventually =/ On Sep ...
2 years, 7 months ago #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 ...
2 years, 7 months ago #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 ...
2 years, 7 months ago #24
willchan
Here are some more comments finally! I have mostly read through all the HttpStreamFactoryImpl::Job integration ...
2 years, 6 months ago #25
James Simonsen
I still need to look into the integration test that Matt was talking about, but ...
2 years, 6 months ago #26
willchan
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 ...
2 years, 6 months ago #27
willchan
HttpPipelinedConnectionImpl is complicated, I am trying to fit everything into my head. Sending more comments. ...
2 years, 6 months ago #28
willchan
Another interesting thing I've noticed about this architecture is that it lets a single request ...
2 years, 6 months ago #29
willchan
The HttpPipelinedConnectionImpl is hard for me to grok. I'm not sure if there's a better ...
2 years, 6 months ago #30
James Simonsen
Done gardening. Time to get this landed... Agreed that this HttpPipelinedConnectionImpl could use another refactoring. ...
2 years, 6 months ago #31
mmenke
A couple more nits. I defer to Will on deciding what should be done before ...
2 years, 6 months ago #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: ...
2 years, 6 months ago #33
willchan
I will take a look later today. James, I am out half of tomorrow and ...
2 years, 6 months ago #34
willchan
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 ...
2 years, 6 months ago #35
willchan
Please update your changelist description to describe the state of the HTTP pipelining implementation as ...
2 years, 6 months ago #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 ...
2 years, 6 months ago #37
James Simonsen
I'll address the rest of the comments tomorrow. I just wanted to answer this question ...
2 years, 6 months ago #38
willchan
On Wed, Oct 12, 2011 at 8:35 PM, <simonjam@chromium.org> wrote: > I'll address the rest ...
2 years, 6 months ago #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: > ...
2 years, 6 months ago #40
mmenke
LGTM. The M16 branch point is late this coming Monday evening/early Tuesday. Mind putting off ...
2 years, 6 months ago #41
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/7289006/142001
2 years, 6 months ago #42
I haz the power (commit-bot)
Can't apply patch for file chrome/common/chrome_switches.cc. While running patch -p1 --forward --force; patching file chrome/common/chrome_switches.cc ...
2 years, 6 months ago #43
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/7289006/146001
2 years, 6 months ago #44
I haz the power (commit-bot)
2 years, 6 months ago #45
Change committed as 106364
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6