Add a force pipelining option to load flags.

Add a force pipelining option to load flags.

Details:
- Add a HttpPipelinedHostForced class for connections with forced requests.
  + Forced requests get their own pipeline and there's only one per host.
  + They always try to pipeline and won't retry if evicted.
  + Only one HttpStreamFactoryImpl::Job runs for all requests to the same
    origin with forced pipelining. All requests will fail if that Job fails.
- Track HttpPipelinedHosts with a Key. Right now that's origin and
  force-pipelining, but it might be expanded to include content type.
- Add a BufferedWriteStreamSocket that wraps a normal socket. It buffers Write()
  calls until a task fires to dispatch the buffer to the underlying socket.

BUG=110794
TEST=net_unittests and unit_tests


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

[James Simonsen, 2012-02-21 23:46:10]

This ended up needing more changes than I expected. Let me know if you'd like me
to split it up into smaller CLs.

[mmenke, 2012-02-23 18:54:58]

Still haven't looked at buffered_write_stream_socket, and I want to look at how
it integrates with the stream factory a bit more closely, but overall, I think
it looks good.

Just a couple nits and suggestions to beef up the unit tests a little.

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/net/http_pipe...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/net/http_pipe...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:141: //
current Histogram.
Why was this working before, but is now causing problems?

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/http_pipeline_view.js (right):

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/resources/net...
chrome/browser/resources/net_internals/http_pipeline_view.js:138:
tablePrinter.addCell(connection.forced);
Suggest you do something to avoid displaying 'undefined' with old log files:

tablePrinter.addCell(connection.forced ? connection.forced : false);

Along with a comment that it'll no longer be needed once M19 hits stable.

http://codereview.chromium.org/9433015/diff/7003/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/base/load_flags_list.h#n...
net/base/load_flags_list.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
nit:  Update year.  Goes for some other files, too.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host.cc
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host.cc:9: HttpPipelinedHost::Key::Key(const
HostPortPair& origin, bool force_pipelining) :
nit:  Colon should go on next line.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host.h:32: class Key {
Doesn't look to me like any of the unit tests beat on this class a whole lot. 
Sure, it's pretty simple, but best to be careful.  Maybe a test with multiple
pipelines for HttpPipelinedHostPool?  Might be a good idea to have a test that
does that, anyways.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_forced.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced.cc:15: using base::Value;
These aren't needed - they're already in base/values.h.  Same goes for
pipelined_host_impl, actually.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced.cc:25: factory_(factory) {
Think you should DCHECK for unexpected key.force_pipelining() values here and in
host_pipelined_force_impl.cc.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_forced.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced.h:32: // to use pipelining.
nit:  Think you should mention that this is intended specifically for testing
pipelining compatibility.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_forced_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced_unittest.cc:30: factory_(new
MockPipelineFactory),  // Owned by host_.
nit:  |host_|

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced_unittest.cc:52: void
ClearTestPipeline(MockPipeline* pipeline) {
This isn't used.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced_unittest.cc:99:
host_->OnPipelineHasCapacity(pipeline);
I suggest in one test you set depth to 1 before calling OnPipelineHasCapacity,
to make sure it behaves as expected, just out of general paranoia.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:69: virtual const Key& key() const OVERRIDE;
Since this is virtual, it really should be GetKey().

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:54: void Initialize() {
Random comment, no action needed:

Is there a reason we don't just do this in SetUp()?

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:788:
data_vector_[0]->RunFor(3);
Why is this 3 and the one below 2?  I'd think they should both be 2.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:827: }
Suggest you also have a test where we get ERR_PIPELINED_EVICTED as the final
result.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
net/http/http_stream_factory_impl.cc:239: host->key()))) {
[optional]: Think this might be a little easier to follow if you just did:

while (ContainsKey(http_pipelining_request_map_, host->key())) {
  HttpPipelinedStream* stream =
      http_pipelined_host_pool_.CreateStreamOnExistingPipeline(origin)));
  if (!stream)
    break;

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:903:
IsExistingPipelineAvailableForKey(*http_pipelining_key_.get())) {
nit:  Suggest you indent this 4 spaces, so it's a little clearer that it's
continuing the previous line.

[James Simonsen, 2012-02-23 23:49:46]

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/net/http_pipe...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/net/http_pipe...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:141: //
current Histogram.
On 2012/02/23 18:54:58, Matt Menke wrote:
> Why was this working before, but is now causing problems?

The old version was broken, but just happened to work. It assumed that once a
histogram was cached, that histogram should always be used. That's incorrect,
the UMA macro will update its cache if sees a different name. So we need to
update our cache in that situation too.

This is tickled now because the evictions finish out of order when we force
pipelining. We receive the evictions before the first request succeeds.

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/http_pipeline_view.js (right):

http://codereview.chromium.org/9433015/diff/7003/chrome/browser/resources/net...
chrome/browser/resources/net_internals/http_pipeline_view.js:138:
tablePrinter.addCell(connection.forced);
On 2012/02/23 18:54:58, Matt Menke wrote:
> Suggest you do something to avoid displaying 'undefined' with old log files:
> 
> tablePrinter.addCell(connection.forced ? connection.forced : false);
> 
> Along with a comment that it'll no longer be needed once M19 hits stable.

Good point. I hadn't thought about that.

http://codereview.chromium.org/9433015/diff/7003/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/base/load_flags_list.h#n...
net/base/load_flags_list.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2012/02/23 18:54:58, Matt Menke wrote:
> nit:  Update year.  Goes for some other files, too.

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host.cc
File net/http/http_pipelined_host.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host.cc:9: HttpPipelinedHost::Key::Key(const
HostPortPair& origin, bool force_pipelining) :
On 2012/02/23 18:54:58, Matt Menke wrote:
> nit:  Colon should go on next line.

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host.h:32: class Key {
On 2012/02/23 18:54:58, Matt Menke wrote:
> Doesn't look to me like any of the unit tests beat on this class a whole lot. 
> Sure, it's pretty simple, but best to be careful.  Maybe a test with multiple
> pipelines for HttpPipelinedHostPool?  Might be a good idea to have a test that
> does that, anyways.

Good point. Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_forced.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced.cc:15: using base::Value;
On 2012/02/23 18:54:58, Matt Menke wrote:
> These aren't needed - they're already in base/values.h.  Same goes for
> pipelined_host_impl, actually.

values.h says not to rely on that in new code.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced.cc:25: factory_(factory) {
On 2012/02/23 18:54:58, Matt Menke wrote:
> Think you should DCHECK for unexpected key.force_pipelining() values here and
in
> host_pipelined_force_impl.cc.

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_forced.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced.h:32: // to use pipelining.
On 2012/02/23 18:54:58, Matt Menke wrote:
> nit:  Think you should mention that this is intended specifically for testing
> pipelining compatibility.

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_forced_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced_unittest.cc:30: factory_(new
MockPipelineFactory),  // Owned by host_.
On 2012/02/23 18:54:58, Matt Menke wrote:
> nit:  |host_|

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced_unittest.cc:52: void
ClearTestPipeline(MockPipeline* pipeline) {
On 2012/02/23 18:54:58, Matt Menke wrote:
> This isn't used.

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_forced_unittest.cc:99:
host_->OnPipelineHasCapacity(pipeline);
On 2012/02/23 18:54:58, Matt Menke wrote:
> I suggest in one test you set depth to 1 before calling OnPipelineHasCapacity,
> to make sure it behaves as expected, just out of general paranoia.

Good idea. Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:69: virtual const Key& key() const OVERRIDE;
On 2012/02/23 18:54:58, Matt Menke wrote:
> Since this is virtual, it really should be GetKey().

Done.

I still have a note to myself to fix these in HttpPipelinedConnection too. It's
always been lower priority than everything else though.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:54: void Initialize() {
On 2012/02/23 18:54:58, Matt Menke wrote:
> Random comment, no action needed:
> 
> Is there a reason we don't just do this in SetUp()?

You've asked that before. It's for the test where we force the socket pool size
to 1. That has to be done before we setup the session. I'll add a comment. :)

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:788:
data_vector_[0]->RunFor(3);
On 2012/02/23 18:54:58, Matt Menke wrote:
> Why is this 3 and the one below 2?  I'd think they should both be 2.

The initial write is also async and is only included in the first one. I added a
comment.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:827: }
On 2012/02/23 18:54:58, Matt Menke wrote:
> Suggest you also have a test where we get ERR_PIPELINED_EVICTED as the final
> result.

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
net/http/http_stream_factory_impl.cc:239: host->key()))) {
On 2012/02/23 18:54:58, Matt Menke wrote:
> [optional]: Think this might be a little easier to follow if you just did:
> 
> while (ContainsKey(http_pipelining_request_map_, host->key())) {
>   HttpPipelinedStream* stream =
>       http_pipelined_host_pool_.CreateStreamOnExistingPipeline(origin)));
>   if (!stream)
>     break;

Done.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:903:
IsExistingPipelineAvailableForKey(*http_pipelining_key_.get())) {
On 2012/02/23 18:54:58, Matt Menke wrote:
> nit:  Suggest you indent this 4 spaces, so it's a little clearer that it's
> continuing the previous line.

Done.

[mmenke, 2012-02-24 00:20:13]

I'll do another round of review tomorrow - probably the final one.

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:54: void Initialize() {
On 2012/02/23 23:49:46, James Simonsen wrote:
> On 2012/02/23 18:54:58, Matt Menke wrote:
> > Random comment, no action needed:
> > 
> > Is there a reason we don't just do this in SetUp()?
> 
> You've asked that before. It's for the test where we force the socket pool
size
> to 1. That has to be done before we setup the session. I'll add a comment. :)

Thanks.  That means I won't have to ask it a third time.  :)

http://codereview.chromium.org/9433015/diff/7003/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:788:
data_vector_[0]->RunFor(3);
On 2012/02/23 23:49:46, James Simonsen wrote:
> On 2012/02/23 18:54:58, Matt Menke wrote:
> > Why is this 3 and the one below 2?  I'd think they should both be 2.
> 
> The initial write is also async and is only included in the first one. I added
a
> comment.

But it's event 0, not event 1, so it doesn't look like it should count.

First call to RunFor runs SetStop(sequence_number_ + seq), which is 3.  Next
call increases it by 1 to 4, then we increase it by 2 to 6, and then we increase
it by 1 to 7.

So for the first call of ExpectResponse(), we're at 3 and increment it to 4, but
the second call we're at 6 and increment it to 7.

Erm...Nevermind.  That makes sense.  Was thinking 3 and 7, rather than 3 and 6.

[mmenke, 2012-02-24 16:36:39]

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:91: protocol_negotiated_))
nit:  Fix indent

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:107: ClientSocketHandle*
dummyConnection =
dummy_connection

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:109: HttpPipelinedStream*
dummyStream =
dummy_stream

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:698: if (!was_new_key &&
http_pipelining_key_->force_pipelining()) {
This looks a little buggy to me.

Suppose we start request A, request A gets a socket synchronously, the request
is deleted, we post a task to call ActivatePipeline.  Then we see another
request B.  The pipeline we created isn't active yet, so
IsExistingPipelineAvailableForKey fails.  Request A has already been serviced,
so |was_new_key| is true, so request B makes it past this point, and could be
assigned another connection synchronously.

As long as we don't already have two idle connections to the host, everything
should work fine, though still seems a little funky.

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:901: if (existing_available_pipeline_
||
nit:  Don't think this check is needed anymore.

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
File net/socket/buffered_write_stream_socket.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
net/socket/buffered_write_stream_socket.cc:114: base::Unretained(this)));
This does not look safe to me.  If we get ERR_IO_PENDING, and then we get
another call to wrapped_socket_->Write(), we realloc the buffer, but the
|wrapped_socket_| holds on to a pointer to the buffer while it's writing, and is
sitll allowed to read from it.  This results in the underlying OS write function
possibly dereferencing a stale pointer.

I believe you need special behavior if BufferedWriteStreamSocket::Write is
called while |wrapped_socket_| is writing to the socket.  You need to return
ERR_IO_PENDING and keep information about the passed in buffer.  Then, when
OnIOComplete is called, you append the cached bytes to send to the io_buffer_
and send them.  Once that's done, you call the callback function that was passed
to write.

Alternatively, you could CHECK that this never happens in
BufferedWriteStreamSocket::Write.  If you do this, should have a big warning in
the header file, and maybe change the class name to be scarier.

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
File net/socket/buffered_write_stream_socket.h (right):

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
net/socket/buffered_write_stream_socket.h:23: class NET_EXPORT_PRIVATE
BufferedWriteStreamSocket : public StreamSocket {
Think think class warrants a short description.

[mmenke, 2012-02-24 16:50:14]

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:698: if (!was_new_key &&
http_pipelining_key_->force_pipelining()) {
On 2012/02/24 16:36:39, Matt Menke wrote:
> This looks a little buggy to me.
> 
> Suppose we start request A, request A gets a socket synchronously, the request
> is deleted, we post a task to call ActivatePipeline.  Then we see another
> request B.  The pipeline we created isn't active yet, so
> IsExistingPipelineAvailableForKey fails.  Request A has already been serviced,
> so |was_new_key| is true, so request B makes it past this point, and could be
> assigned another connection synchronously.
> 
> As long as we don't already have two idle connections to the host, everything
> should work fine, though still seems a little funky.

Sorry, forgot that HttpPipelinedHostForced::IsExistingPipelineAvailable should
return true, even for inactive pipelines, though I'm not sure how happily a
pipelined connection will handle a new request when not active.

[James Simonsen, 2012-02-24 23:58:51]

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:91: protocol_negotiated_))
On 2012/02/24 16:36:39, Matt Menke wrote:
> nit:  Fix indent

Done.

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:107: ClientSocketHandle*
dummyConnection =
On 2012/02/24 16:36:39, Matt Menke wrote:
> dummy_connection

Done.

http://codereview.chromium.org/9433015/diff/8003/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:109: HttpPipelinedStream*
dummyStream =
On 2012/02/24 16:36:39, Matt Menke wrote:
> dummy_stream

Done.

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:698: if (!was_new_key &&
http_pipelining_key_->force_pipelining()) {
On 2012/02/24 16:50:14, Matt Menke wrote:
> On 2012/02/24 16:36:39, Matt Menke wrote:
> > This looks a little buggy to me.
> > 
> > Suppose we start request A, request A gets a socket synchronously, the
request
> > is deleted, we post a task to call ActivatePipeline.  Then we see another
> > request B.  The pipeline we created isn't active yet, so
> > IsExistingPipelineAvailableForKey fails.  Request A has already been
serviced,
> > so |was_new_key| is true, so request B makes it past this point, and could
be
> > assigned another connection synchronously.
> > 
> > As long as we don't already have two idle connections to the host,
everything
> > should work fine, though still seems a little funky.
> 
> Sorry, forgot that HttpPipelinedHostForced::IsExistingPipelineAvailable should
> return true, even for inactive pipelines, though I'm not sure how happily a
> pipelined connection will handle a new request when not active.

It won't mind. Pipelines don't enforce activation. The HttpPipelinedHost
enforces it (if applicable).

The whole activation thing was to prevent other requests from cutting in line.
So request A should always get a chance to SendRequest() first. If it does that
synchronously, then the pipeline is activated synchronously. If it doesn't, then
the pipeline activates on a task.

In your example, A would've had its chance to SendRequest() synchronously. If it
hasn't sent already, then a task is already posted to activate it. B is just
beating that task out, but the end result will be the same.

This is definitely subtle (and I hadn't considered your scenario until you
brought it up). I wonder if there's a better way to write this code so this case
is more obvious. Do you have any ideas?

The problem is I don't have any way of knowing that A _didn't_ call
SendRequest() until the ActivatePipeline task fires. If I did know, I would
enforce activation to prevent this breaking during a refactoring. And then the
pipeline would already be active when B tries to use it and it'd be obviously
okay.

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:901: if (existing_available_pipeline_
||
On 2012/02/24 16:50:14, Matt Menke wrote:
> On 2012/02/24 16:36:39, Matt Menke wrote:
> > This looks a little buggy to me.
> > 
> > Suppose we start request A, request A gets a socket synchronously, the
request
> > is deleted, we post a task to call ActivatePipeline.  Then we see another
> > request B.  The pipeline we created isn't active yet, so
> > IsExistingPipelineAvailableForKey fails.  Request A has already been
serviced,
> > so |was_new_key| is true, so request B makes it past this point, and could
be
> > assigned another connection synchronously.
> > 
> > As long as we don't already have two idle connections to the host,
everything
> > should work fine, though still seems a little funky.
> 
> Sorry, forgot that HttpPipelinedHostForced::IsExistingPipelineAvailable should
> return true, even for inactive pipelines, though I'm not sure how happily a
> pipelined connection will handle a new request when not active.

It won't mind. Activation isn't enforced by the pipeline.

The whole activation thing was to prevent other requests from cutting in line.
So the request A should always get a chance to SendRequest() first. If it does
that synchronously, then the pipeline is activated synchronously. If it doesn't,
then the pipeline activates on a task.

In your example, A would've had its chance to SendRequest() synchronously. If it
hasn't sent already, then a task is already posted to activate it. B is just
beating that task out, but the end result will be the same.

This is definitely subtle (and I hadn't considered your scenario until you
brought it up). I wonder if there's a better way to write this code so this case
is more obvious.

The problem is I don't have any way of knowing that A _didn't_ call
SendRequest() until the ActivatePipeline task fires. If I did know, I would
enforce activation to prevent this breaking during a refactoring.

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
File net/socket/buffered_write_stream_socket.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
net/socket/buffered_write_stream_socket.cc:114: base::Unretained(this)));
On 2012/02/24 16:36:39, Matt Menke wrote:
> This does not look safe to me.  If we get ERR_IO_PENDING, and then we get
> another call to wrapped_socket_->Write(), we realloc the buffer, but the
> |wrapped_socket_| holds on to a pointer to the buffer while it's writing, and
is
> sitll allowed to read from it.  This results in the underlying OS write
function
> possibly dereferencing a stale pointer.
> 
> I believe you need special behavior if BufferedWriteStreamSocket::Write is
> called while |wrapped_socket_| is writing to the socket.  You need to return
> ERR_IO_PENDING and keep information about the passed in buffer.  Then, when
> OnIOComplete is called, you append the cached bytes to send to the io_buffer_
> and send them.  Once that's done, you call the callback function that was
passed
> to write.
> 
> Alternatively, you could CHECK that this never happens in
> BufferedWriteStreamSocket::Write.  If you do this, should have a big warning
in
> the header file, and maybe change the class name to be scarier.

Nice catch!

I went with a backup buffer. It's the same sort of idea as double buffering for
a video display, except that we append instead of swap.

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
File net/socket/buffered_write_stream_socket.h (right):

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
net/socket/buffered_write_stream_socket.h:23: class NET_EXPORT_PRIVATE
BufferedWriteStreamSocket : public StreamSocket {
On 2012/02/24 16:36:39, Matt Menke wrote:
> Think think class warrants a short description.

Done.

[mmenke, 2012-02-25 00:32:43]

LGTM, as-is.  Suggest you add one unit test, though.

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:698: if (!was_new_key &&
http_pipelining_key_->force_pipelining()) {
On 2012/02/24 23:58:51, James Simonsen wrote:
> On 2012/02/24 16:50:14, Matt Menke wrote:
> > On 2012/02/24 16:36:39, Matt Menke wrote:
> > > This looks a little buggy to me.
> > > 
> > > Suppose we start request A, request A gets a socket synchronously, the
> request
> > > is deleted, we post a task to call ActivatePipeline.  Then we see another
> > > request B.  The pipeline we created isn't active yet, so
> > > IsExistingPipelineAvailableForKey fails.  Request A has already been
> serviced,
> > > so |was_new_key| is true, so request B makes it past this point, and could
> be
> > > assigned another connection synchronously.
> > > 
> > > As long as we don't already have two idle connections to the host,
> everything
> > > should work fine, though still seems a little funky.
> > 
> > Sorry, forgot that HttpPipelinedHostForced::IsExistingPipelineAvailable
should
> > return true, even for inactive pipelines, though I'm not sure how happily a
> > pipelined connection will handle a new request when not active.
> 
> It won't mind. Pipelines don't enforce activation. The HttpPipelinedHost
> enforces it (if applicable).
> 
> The whole activation thing was to prevent other requests from cutting in line.
> So request A should always get a chance to SendRequest() first. If it does
that
> synchronously, then the pipeline is activated synchronously. If it doesn't,
then
> the pipeline activates on a task.
> 
> In your example, A would've had its chance to SendRequest() synchronously. If
it
> hasn't sent already, then a task is already posted to activate it. B is just
> beating that task out, but the end result will be the same.
> 
> This is definitely subtle (and I hadn't considered your scenario until you
> brought it up). I wonder if there's a better way to write this code so this
case
> is more obvious. Do you have any ideas?
> 
> The problem is I don't have any way of knowing that A _didn't_ call
> SendRequest() until the ActivatePipeline task fires. If I did know, I would
> enforce activation to prevent this breaking during a refactoring. And then the
> pipeline would already be active when B tries to use it and it'd be obviously
> okay.

I want to think about this a little, see if we can some up with something
clearer, but think we can live with this for now.  A comment might be nice, if
you can avoid making it 500 lines of text.  :)

Looks like we also handle the usable_ = false case correctly, which I was also
worried about.

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
File net/socket/buffered_write_stream_socket.cc (right):

http://codereview.chromium.org/9433015/diff/8003/net/socket/buffered_write_st...
net/socket/buffered_write_stream_socket.cc:114: base::Unretained(this)));
On 2012/02/24 23:58:51, James Simonsen wrote:
> On 2012/02/24 16:36:39, Matt Menke wrote:
> > This does not look safe to me.  If we get ERR_IO_PENDING, and then we get
> > another call to wrapped_socket_->Write(), we realloc the buffer, but the
> > |wrapped_socket_| holds on to a pointer to the buffer while it's writing,
and
> is
> > sitll allowed to read from it.  This results in the underlying OS write
> function
> > possibly dereferencing a stale pointer.
> > 
> > I believe you need special behavior if BufferedWriteStreamSocket::Write is
> > called while |wrapped_socket_| is writing to the socket.  You need to return
> > ERR_IO_PENDING and keep information about the passed in buffer.  Then, when
> > OnIOComplete is called, you append the cached bytes to send to the
io_buffer_
> > and send them.  Once that's done, you call the callback function that was
> passed
> > to write.
> > 
> > Alternatively, you could CHECK that this never happens in
> > BufferedWriteStreamSocket::Write.  If you do this, should have a big warning
> in
> > the header file, and maybe change the class name to be scarier.
> 
> Nice catch!
> 
> I went with a backup buffer. It's the same sort of idea as double buffering
for
> a video display, except that we append instead of swap.

Great, I much prefer this approach..

http://codereview.chromium.org/9433015/diff/18012/net/socket/buffered_write_s...
File net/socket/buffered_write_stream_socket.cc (right):

http://codereview.chromium.org/9433015/diff/18012/net/socket/buffered_write_s...
net/socket/buffered_write_stream_socket.cc:140:
backup_buffer_->RemainingCapacity());
Don't think any of the tests currently exercise this.

Think all you'll need to do is start an async write, run all pending events, and
then start another async write or two.

[James Simonsen, 2012-02-25 03:11:37]

I managed to make it a 4-line comment. I hope that's good enough. I'll let the
try bots run and land this Monday morning.

http://codereview.chromium.org/9433015/diff/18012/net/socket/buffered_write_s...
File net/socket/buffered_write_stream_socket.cc (right):

http://codereview.chromium.org/9433015/diff/18012/net/socket/buffered_write_s...
net/socket/buffered_write_stream_socket.cc:140:
backup_buffer_->RemainingCapacity());
On 2012/02/25 00:32:43, Matt Menke wrote:
> Don't think any of the tests currently exercise this.
> 
> Think all you'll need to do is start an async write, run all pending events,
and
> then start another async write or two.

Done. It was covered by the WriteWhileBlocked test, but I added an extra step to
it for fun. Turns out it caught a bug where were appending to the wrong part of
the buffer, so I fixed that too.

[commit-bot: I haz the power, 2012-02-27 23:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9433015/25002

[commit-bot: I haz the power, 2012-02-27 23:27:42]

Presubmit check for 9433015-25002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  net/http/http_pipelined_host_test_util.cc \
  net/http/http_pipelined_host_test_util.h

Presubmit checks took 3.0s to calculate.

[commit-bot: I haz the power, 2012-02-27 23:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9433015/24006

[commit-bot: I haz the power, 2012-02-28 18:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9433015/26034

[commit-bot: I haz the power, 2012-02-28 23:29:09]

Try job failure for 9433015-26034 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[willchan no longer on Chromium, 2012-02-28 23:44:21]

Sorry, I didn't look at this earlier. I have to say I somewhat disagree with
this approach. Don't let that stop you though, I'm comfortable with you guys
owning this. I just want to voice my opinion so you guys can consider it.

I'm mildly concerned that this load flag solution may add undesirable complexity
to HttpStreamFactoryImpl. I also dislike the granularity of a load flag. I sort
of think that we should be using a separate URLRequestContext dedicated solely
to this, and owned by the HTTP pipelining client. It seems like it'd be cool if
we could construct a HttpNetworkLayer with a special parameter, that gets
plumbed through to the HttpStreamFactoryImpl that puts it in a pipelining only
mode. This would help silo off this special behavior. I'm concerned that with
the new load flag, that really is only for this http pipelining compatibility
use case (right?), it gives the net/ user a long enough rope to hang himself
with, if he abuses that load flag due to incompetence.

That's just my two cents. Cheers.

[James Simonsen, 2012-03-01 01:11:24]

Fortunately, the commit-queue was sufficiently borked that this didn't land
before your comment came through. It wasn't a lot of extra work, so I just went
ahead and did it.

The HttpPipelinedHost::Key seems useless now, but I'd like to keep it around
since we may need it in the future for other purposes (pipelining only with like
MIME types, which I've heard Firefox does).

[mmenke, 2012-03-01 16:50:30]

http://codereview.chromium.org/9433015/diff/35001/net/http/http_network_sessi...
File net/http/http_network_session.h (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_network_sessi...
net/http/http_network_session.h:146: Params params() const { return params_; }
Nit:  const Params&

http://codereview.chromium.org/9433015/diff/35001/net/http/http_pipelined_net...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_pipelined_net...
net/http/http_pipelined_network_transaction_unittest.cc:757:
TEST_F(HttpPipelinedNetworkTransactionTest, ForcedPipelineSharesConnection) {
I think we might want a connection that connects asynchronously, to make sure we
don't try and open a second connection.

http://codereview.chromium.org/9433015/diff/35001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:703: return ERR_IO_PENDING;
I'm not sure this address's Will's concern about HttpStreamFactoryImpl's
complexity.

[James Simonsen, 2012-03-01 18:49:29]

http://codereview.chromium.org/9433015/diff/35001/net/http/http_network_sessi...
File net/http/http_network_session.h (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_network_sessi...
net/http/http_network_session.h:146: Params params() const { return params_; }
On 2012/03/01 16:50:30, Matt Menke wrote:
> Nit:  const Params&

Done.

http://codereview.chromium.org/9433015/diff/35001/net/http/http_pipelined_net...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_pipelined_net...
net/http/http_pipelined_network_transaction_unittest.cc:757:
TEST_F(HttpPipelinedNetworkTransactionTest, ForcedPipelineSharesConnection) {
On 2012/03/01 16:50:30, Matt Menke wrote:
> I think we might want a connection that connects asynchronously, to make sure
we
> don't try and open a second connection.

I've got one in an upcoming CL. While I was waiting for this one, I went ahead
and finished a bunch of other stuff.

http://codereview.chromium.org/9433015/diff/35001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:703: return ERR_IO_PENDING;
On 2012/03/01 16:50:30, Matt Menke wrote:
> I'm not sure this address's Will's concern about HttpStreamFactoryImpl's
> complexity.

Any ideas?

The "right" thing to do would be to write a whole new factory, job, and request.
90% of the code would be the same though and it seems silly to copy & paste it.

Trying to share that code with common base classes would just confuse the normal
code path, which is a bad trade-off. I'd rather the special case be the
confusing one (which is what we have now).

Unless there's a better idea, I think the current code is the best of the
options.

[mmenke, 2012-03-01 19:10:09]

LGTM

http://codereview.chromium.org/9433015/diff/35001/net/http/http_pipelined_net...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_pipelined_net...
net/http/http_pipelined_network_transaction_unittest.cc:757:
TEST_F(HttpPipelinedNetworkTransactionTest, ForcedPipelineSharesConnection) {
On 2012/03/01 18:49:29, James Simonsen wrote:
> On 2012/03/01 16:50:30, Matt Menke wrote:
> > I think we might want a connection that connects asynchronously, to make
sure
> we
> > don't try and open a second connection.
> 
> I've got one in an upcoming CL. While I was waiting for this one, I went ahead
> and finished a bunch of other stuff.

Great!

http://codereview.chromium.org/9433015/diff/35001/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/9433015/diff/35001/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:703: return ERR_IO_PENDING;
On 2012/03/01 18:49:29, James Simonsen wrote:
> On 2012/03/01 16:50:30, Matt Menke wrote:
> > I'm not sure this address's Will's concern about HttpStreamFactoryImpl's
> > complexity.
> 
> Any ideas?
> 
> The "right" thing to do would be to write a whole new factory, job, and
request.
> 90% of the code would be the same though and it seems silly to copy & paste
it.
> 
> Trying to share that code with common base classes would just confuse the
normal
> code path, which is a bad trade-off. I'd rather the special case be the
> confusing one (which is what we have now).
> 
> Unless there's a better idea, I think the current code is the best of the
> options.

I was thinking maybe a subclass...Not sure how workable that would be, though.

[commit-bot: I haz the power, 2012-03-01 19:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9433015/48015

[commit-bot: I haz the power, 2012-03-01 19:14:29]

Can't apply patch for file
chrome/browser/resources/net_internals/http_pipeline_view.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/net_internals/http_pipeline_view.js
Hunk #1 FAILED at 1.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/resources/net_internals/http_pipeline_view.js.rej

[James Simonsen, 2012-03-01 19:19:55]

On 2012/03/01 19:10:09, Matt Menke wrote:
> I was thinking maybe a subclass...Not sure how workable that would be, though.

I'm going to land what we have for now. I've added a note on my TODO list to
revisit this later.

I think fixing the API was the more urgent part of Will's comments. The
HttpStreamFactory can be refactored later without affecting net/ users.

Like I said a while ago, there's a tiny possibility none of this code will be
needed in practice. I'd like to get the client running asap. Once we have the
data, we can decide where best to place our effort.

[mmenke, 2012-03-01 19:20:47]

On 2012/03/01 19:19:55, James Simonsen wrote:
> On 2012/03/01 19:10:09, Matt Menke wrote:
> > I was thinking maybe a subclass...Not sure how workable that would be,
though.
> 
> I'm going to land what we have for now. I've added a note on my TODO list to
> revisit this later.
> 
> I think fixing the API was the more urgent part of Will's comments. The
> HttpStreamFactory can be refactored later without affecting net/ users.
> 
> Like I said a while ago, there's a tiny possibility none of this code will be
> needed in practice. I'd like to get the client running asap. Once we have the
> data, we can decide where best to place our effort.

SGTM.

[willchan no longer on Chromium, 2012-03-01 19:22:59]

SGTM, thanks guys.

On Thu, Mar 1, 2012 at 11:20 AM, <mmenke@chromium.org> wrote:

> On 2012/03/01 19:19:55, James Simonsen wrote:
>
>> On 2012/03/01 19:10:09, Matt Menke wrote:
>> > I was thinking maybe a subclass...Not sure how workable that would be,
>>
> though.
>
>  I'm going to land what we have for now. I've added a note on my TODO list
>> to
>> revisit this later.
>>
>
>  I think fixing the API was the more urgent part of Will's comments. The
>> HttpStreamFactory can be refactored later without affecting net/ users.
>>
>
>  Like I said a while ago, there's a tiny possibility none of this code
>> will be
>> needed in practice. I'd like to get the client running asap. Once we have
>> the
>> data, we can decide where best to place our effort.
>>
>
> SGTM.
>
>
http://codereview.chromium.**org/9433015/<http://codereview.chromium.org/9433...
>

[commit-bot: I haz the power, 2012-03-01 19:52:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9433015/49006

[commit-bot: I haz the power, 2012-03-01 21:31:36]

Change committed as 124487