Retry upon 421 status code without IP pooling.

Retry upon 421 status code without IP pooling.

Implement logic in HttpNetworkTransaction to retry a request at most
once with IP pooling disabled if status code 421 Misdirected Request is
received in the response.

Add ERR_MISDIRECTED_REQUEST, which is used within
HttpNetworkTransaction, and is also logged in NetLog event
HTTP_TRANSACTION_RESTART_AFTER_ERROR.

Plumb |enable_ip_based_pooling| from HttpNetworkTransaction through
HttpStreamRequest::RequestStream() through
HttpStreamFactoryImpl::JobController::JobController() through
HttpStreamFactoryImpl::JobFactory::CreateJob() through
HttpStreamFactoryImpl::Job::Job() to
SpdySessionPool::FindAvailableSession().

Add HttpNetworkTransactionTest.

Add extra check in SpdySessionPoolTest::RunIPPoolingTest().

BUG=546991
Review-Url: https://codereview.chromium.org/2771263002
Cr-Commit-Position: refs/heads/master@{#460996}
Committed: https://chromium.googlesource.com/chromium/src/+/8016c1f6899aaf53e3cc81d29cc9c8306b6e9927

[Bence, 2017-03-24 17:21:30]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 17:22:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 18:46:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 18:46:02]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-03-24 18:47:35]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2017-03-24 18:47:35]

Ryan: PTAL.  Thank you.

[Ryan Hamilton, 2017-03-24 21:44:05]

rch@chromium.org changed reviewers:
+ zhongyi@chromium.org

[Ryan Hamilton, 2017-03-24 21:44:06]

zhongyi: Can you do a "shadow" review of this CL? Once you're happy I'll do an
OWNERS review.

[Zhongyi Shi, 2017-03-27 15:05:53]

Sorry about the delay. I'll review today.

[Zhongyi Shi, 2017-03-29 03:05:06]

Looks good, thanks for the patience!

I was wondering though, since the enable_ip_based_pooling is not always true in
HttpStreamFactoryImpl and its friends. Should we add some test coverage in
HttpStreamFactoryImplTest, JobControllerTest or some such?

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:1545: // If this is the second try, just
give up.
nit: it might be worth noting in the comments saying disable
enable_ip_based_pooling if it's retry for MISDIRECTED_REQUEST.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13610:
session_deps_.host_resolver->rules()->AddRule("mail.example.com", ip_addr);
optional nit: s/mail.example.com/mail.example.org, totally optional. 
I think we might need a clean-up in net unittests.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13686: // Preload mail.example.com
into HostCache.
nit: This seemed to be part of the test setting up, which doesn't have to happen
after the first request complete? If so, it might be cleaner to move this
upfront before we start the first transaction.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13704: rv =
trans2.Start(&request2, callback.callback(), NetLogWithSource());
Seems like we are only verifying the second request succeeds in this test. It
might be better to replace the NetLogWithSource() by TestNetLog so that you
could verify that MISDIRECTED_RESPONSE is processed.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: spdy_session_key, origin_url,
enable_ip_based_pooling, net_log)
I thought we could just use the enable_ip_based_pooling_ here and you don't need
to pass the boolean over since the only caller is the same Job? Any reason to
pass this boolean explicitly?

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1007: spdy_session_key, origin_url_,
enable_ip_based_pooling_)
ditto

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:437:
enable_ip_based_pooling_, job->net_log().net_log()));
This is trying to start a alternative proxy job, just to confirm, we also want
to disable enable_ip_based_pooling on the alt proxy job right?

[Bence, 2017-03-29 16:46:08]

On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> Looks good, thanks for the patience!
> 
> I was wondering though, since the enable_ip_based_pooling is not always true
in
> HttpStreamFactoryImpl and its friends. Should we add some test coverage in
> HttpStreamFactoryImplTest, JobControllerTest or some such?
> 

I think it would be a great idea if there was an easy way to do this.  To write
a meaningful test, one needs a SpdySession in the pool with the same IP address,
then issue a second request which would instantiate a new JobController.  I do
not think this fits in either with the current HttpStreamFactoryImplTests or
JobControllerTests, so I figured the best place to write a test is in
HttpNetworkTransactionTest.

Or one could write a trivial JobControllerTest to check if the correct Boolean
value for enable_ip_based_pooling was passed to the Job that the controller
created, but I do not see too much value in this.

Please let me know if you can think of a better kind of
HttpStreamFactoryImplTest or JobControllerTest, I'm happy to add one if it makes
sense.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:1545: // If this is the second try, just
give up.
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> nit: it might be worth noting in the comments saying disable
> enable_ip_based_pooling if it's retry for MISDIRECTED_REQUEST.

Done.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13610:
session_deps_.host_resolver->rules()->AddRule("mail.example.com", ip_addr);
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> optional nit: http://s/mail.example.com/mail.example.org, totally optional. 
> I think we might need a clean-up in net unittests.

Done.

BTW RFC2606 Section 3 allows both example.com and example.org, and
spdy_pooling.pem covers www.example.org, mail.example.org, and mail.example.com,
so it does not matter much, but if you want to clean up net unittests, I'm happy
to approve that change.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13686: // Preload mail.example.com
into HostCache.
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> nit: This seemed to be part of the test setting up, which doesn't have to
happen
> after the first request complete? If so, it might be cleaner to move this
> upfront before we start the first transaction. 

Done.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13704: rv =
trans2.Start(&request2, callback.callback(), NetLogWithSource());
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> Seems like we are only verifying the second request succeeds in this test. It
> might be better to replace the NetLogWithSource() by TestNetLog so that you
> could verify that MISDIRECTED_RESPONSE is processed.

Excellent idea, thank you!  Done.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: spdy_session_key, origin_url,
enable_ip_based_pooling, net_log)
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> I thought we could just use the enable_ip_based_pooling_ here and you don't
need
> to pass the boolean over since the only caller is the same Job? Any reason to
> pass this boolean explicitly?

Yes, this method is static.  That's why |spdy_session_pool| and |origin_url| are
also bound to it.

Do you think it would be worthwhile to change this method to be non-static and
bind it to the class instance?  If so, I might prefer to do it in a separate CL,
either before or after this one.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1007: spdy_session_key, origin_url_,
enable_ip_based_pooling_)
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> ditto

ditto

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:437:
enable_ip_based_pooling_, job->net_log().net_log()));
On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> This is trying to start a alternative proxy job, just to confirm, we also want
> to disable enable_ip_based_pooling on the alt proxy job right? 

That is correct.

In fact, in a follow-up CL, I will disable alternative jobs for retries after a
421 response.

[Bence, 2017-03-29 16:46:55]

PTAL.  Thank you.

[Zhongyi Shi, 2017-03-30 22:33:02]

Thanks for the patience, lgtm.

The rationale of not doing JobControllerTest sounded reasonable to me. I was
thinking manually add entries to issue some requests to pull an active session
into spdy_session_pool, then test against disabled ip-based pooling, the code
path is not complicated and since it is already covered in
HttpNetworkTransaction test, I think JobController doesn't have to test that
again. =）

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:13610:
session_deps_.host_resolver->rules()->AddRule("mail.example.com", ip_addr);
On 2017/03/29 16:46:08, Bence wrote:
> On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> > optional nit: http://s/mail.example.com/mail.example.org, totally optional. 
> > I think we might need a clean-up in net unittests.
> 
> Done.
> 
> BTW RFC2606 Section 3 allows both http://example.com and http://example.org,
and
> spdy_pooling.pem covers http://www.example.org, http://mail.example.org, and
http://mail.example.com,
> so it does not matter much, but if you want to clean up net unittests, I'm
happy
> to approve that change.

Acknowledged.

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:550: spdy_session_key, origin_url,
enable_ip_based_pooling, net_log)
On 2017/03/29 16:46:08, Bence wrote:
> On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> > I thought we could just use the enable_ip_based_pooling_ here and you don't
> need
> > to pass the boolean over since the only caller is the same Job? Any reason
to
> > pass this boolean explicitly?
> 
> Yes, this method is static.  That's why |spdy_session_pool| and |origin_url|
are
> also bound to it.
> 
> Do you think it would be worthwhile to change this method to be non-static and
> bind it to the class instance?  If so, I might prefer to do it in a separate
CL,
> either before or after this one.

Acknowledged. I see, I didn't realize it's a static function. I am fine with
either one as you choose then!

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2771263002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:437:
enable_ip_based_pooling_, job->net_log().net_log()));
On 2017/03/29 16:46:08, Bence wrote:
> On 2017/03/29 03:05:06, Zhongyi Shi wrote:
> > This is trying to start a alternative proxy job, just to confirm, we also
want
> > to disable enable_ip_based_pooling on the alt proxy job right? 
> 
> That is correct.
> 
> In fact, in a follow-up CL, I will disable alternative jobs for retries after
a
> 421 response.

SGTM!

https://codereview.chromium.org/2771263002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2771263002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:13725: EXPECT_THAT(rv,
IsError(ERR_MISDIRECTED_REQUEST));
awesome!

[Bence, 2017-03-30 22:53:03]

Ryan: PTAL.
Zhongyi: Thank you for the review.

[Bence, 2017-03-30 23:01:32]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 23:03:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Hamilton, 2017-03-30 23:51:07]

lgtm

Should we also do this for QUIC too?

https://codereview.chromium.org/2771263002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2771263002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1544: case ERR_MISDIRECTED_REQUEST:
It's a bit weird that ERR_MISDIRECTED_REQUEST is a new error code which is only
used in a single .cc file. It seems like we could just do the retry inline when
we find the 421 status code. But HTTP_TRANSACTION_RESTART_AFTER_ERROR seems
logical, so adding a new error seems reasonable.

[Bence, 2017-03-31 00:09:33]

Thank you both for the review.

As for QUIC, I am not sure if there will be a QUIC server sending 421, also I am
not sure what is the plan with the ORIGIN frame which will give servers a way to
discourage the client from issuing a request that the server cannot serve in the
first place.

https://codereview.chromium.org/2771263002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/2771263002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1544: case ERR_MISDIRECTED_REQUEST:
On 2017/03/30 23:51:07, Ryan Hamilton wrote:
> It's a bit weird that ERR_MISDIRECTED_REQUEST is a new error code which is
only
> used in a single .cc file. It seems like we could just do the retry inline
when
> we find the 421 status code. But HTTP_TRANSACTION_RESTART_AFTER_ERROR seems
> logical, so adding a new error seems reasonable.

Yes, I absolutely agree with you.  I was thinking about inlining, but decided
that introducing a new net error code is worth the clarity we get in NetLog.

[commit-bot: I haz the power, 2017-03-31 00:38:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 00:38:26]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bence, 2017-03-31 01:29:46]

The CQ bit was checked by bnc@chromium.org

[Bence, 2017-03-31 01:29:46]

The patchset sent to the CQ was uploaded after l-g-t-m from zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2771263002/#ps40001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-31 01:30:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 02:11:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490923786344910,
"parent_rev": "6ad1835ebdf60d5363f0df85437f34a4c3e46052", "commit_rev":
"8016c1f6899aaf53e3cc81d29cc9c8306b6e9927"}

[commit-bot: I haz the power, 2017-03-31 02:12:22]

Description was changed from

==========
Retry upon 421 status code without IP pooling.

Implement logic in HttpNetworkTransaction to retry a request at most
once with IP pooling disabled if status code 421 Misdirected Request is
received in the response.

Add ERR_MISDIRECTED_REQUEST, which is used within
HttpNetworkTransaction, and is also logged in NetLog event
HTTP_TRANSACTION_RESTART_AFTER_ERROR.

Plumb |enable_ip_based_pooling| from HttpNetworkTransaction through
HttpStreamRequest::RequestStream() through
HttpStreamFactoryImpl::JobController::JobController() through
HttpStreamFactoryImpl::JobFactory::CreateJob() through
HttpStreamFactoryImpl::Job::Job() to
SpdySessionPool::FindAvailableSession().

Add HttpNetworkTransactionTest.

Add extra check in SpdySessionPoolTest::RunIPPoolingTest().

BUG=546991
==========

to

==========
Retry upon 421 status code without IP pooling.

Implement logic in HttpNetworkTransaction to retry a request at most
once with IP pooling disabled if status code 421 Misdirected Request is
received in the response.

Add ERR_MISDIRECTED_REQUEST, which is used within
HttpNetworkTransaction, and is also logged in NetLog event
HTTP_TRANSACTION_RESTART_AFTER_ERROR.

Plumb |enable_ip_based_pooling| from HttpNetworkTransaction through
HttpStreamRequest::RequestStream() through
HttpStreamFactoryImpl::JobController::JobController() through
HttpStreamFactoryImpl::JobFactory::CreateJob() through
HttpStreamFactoryImpl::Job::Job() to
SpdySessionPool::FindAvailableSession().

Add HttpNetworkTransactionTest.

Add extra check in SpdySessionPoolTest::RunIPPoolingTest().

BUG=546991

Review-Url: https://codereview.chromium.org/2771263002
Cr-Commit-Position: refs/heads/master@{#460996}
Committed:
https://chromium.googlesource.com/chromium/src/+/8016c1f6899aaf53e3cc81d29cc9...
==========

[commit-bot: I haz the power, 2017-03-31 02:12:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8016c1f6899aaf53e3cc81d29cc9...