Retry HttpNetworkTransactions upon receipt of ERR_NETWORK_CHANGED during host resolution and …

Retry HttpNetworkTransactions upon receipt of ERR_NETWORK_CHANGED during host resolution and connecting. By limiting the retrying to only during host resolution and connecting we avoid ever duplicating request side-effects.

BUG=166593, 249346

[pauljensen, 2014-01-28 17:48:18]

Matt, What do you think of the general idea here?  If you like the idea I'll
work to finish off the changes by adding tests.

[mmenke, 2014-01-28 17:58:15]

On 2014/01/28 17:48:18, pauljensen wrote:
> Matt, What do you think of the general idea here?  If you like the idea I'll
> work to finish off the changes by adding tests.

So this should catch for the socket pool ERR_NETWORK_CHANGED and the host
resolver one.

Potential problem:  Is it possible for us to get, say, one from the
HostResolver, start a new request, and then get another error from the socket
pool?  I'm assuming you've tested enough to know that doesn't happen, but
relying on there being an async event somewhere in there so that doesn't happen
seems regression prone to me.

Both the SocketPools and the HostResolver are complicated enough that I think
that retrying in the HttpNetworkTransaction makes sense.

Are there any SPDY/QUIC cases this can catch?  They presumably mostly cause the
error after streams have been created, but I'd still like to know.

[pauljensen, 2014-01-30 03:46:20]

On 2014/01/28 17:58:15, mmenke wrote:
> On 2014/01/28 17:48:18, pauljensen wrote:
> > Matt, What do you think of the general idea here?  If you like the idea I'll
> > work to finish off the changes by adding tests.
> 
> So this should catch for the socket pool ERR_NETWORK_CHANGED and the host
> resolver one.
> 
> Potential problem:  Is it possible for us to get, say, one from the
> HostResolver, start a new request, and then get another error from the socket
> pool?  I'm assuming you've tested enough to know that doesn't happen, but
> relying on there being an async event somewhere in there so that doesn't
happen
> seems regression prone to me.
Good point, I've added a PostTask() delay in there so all network observers can
be flushed from the task queue; this is what I recommended for the URLFetcher
restarting on ERR_NETWORK_CHANGED also.

> 
> Both the SocketPools and the HostResolver are complicated enough that I think
> that retrying in the HttpNetworkTransaction makes sense.
> 
> Are there any SPDY/QUIC cases this can catch?  They presumably mostly cause
the
> error after streams have been created, but I'd still like to know.
I think this could catch initial SpdySession establishment DNS and connect()
failures.  For QUIC I imagine this could catch ERR_NETWORK_CHANGED during DNS,
I'll have to investigate what QUIC does for network changes during "connecting".
 FYI I'm working to reduce ERR_NETWORK_CHANGED from Spdy here:
http://crrev.com/129873010

[pauljensen, 2014-02-06 03:49:04]

On 2014/01/30 03:46:20, pauljensen wrote:
> On 2014/01/28 17:58:15, mmenke wrote:
> > On 2014/01/28 17:48:18, pauljensen wrote:
> > > Matt, What do you think of the general idea here?  If you like the idea
I'll
> > > work to finish off the changes by adding tests.
> > 
> > So this should catch for the socket pool ERR_NETWORK_CHANGED and the host
> > resolver one.
> > 
> > Potential problem:  Is it possible for us to get, say, one from the
> > HostResolver, start a new request, and then get another error from the
socket
> > pool?  I'm assuming you've tested enough to know that doesn't happen, but
> > relying on there being an async event somewhere in there so that doesn't
> happen
> > seems regression prone to me.
> Good point, I've added a PostTask() delay in there so all network observers
can
> be flushed from the task queue; this is what I recommended for the URLFetcher
> restarting on ERR_NETWORK_CHANGED also.
> 
> > 
> > Both the SocketPools and the HostResolver are complicated enough that I
think
> > that retrying in the HttpNetworkTransaction makes sense.
> > 
> > Are there any SPDY/QUIC cases this can catch?  They presumably mostly cause
> the
> > error after streams have been created, but I'd still like to know.
> I think this could catch initial SpdySession establishment DNS and connect()
> failures.  For QUIC I imagine this could catch ERR_NETWORK_CHANGED during DNS,
> I'll have to investigate what QUIC does for network changes during
"connecting".
>  FYI I'm working to reduce ERR_NETWORK_CHANGED from Spdy here:
> http://crrev.com/129873010

I did some more investigation/thinking about the SPDY and QUIC cases.  Taking
into account my http://crrev.com/129873010 change, we really shouldn't be
getting ERR_NETWORK_CHANGED from requests using SPDY (or QUIC if similar changes
can be implemented) once the SpdySession is established.  So the only
ERR_NETWORK_CHANGED errors we do care about will surface during SpdySession
establishment.  I believe SPDY (and hence the choice of establishing a
SpdySession) is selected in one of two ways:  for HTTP connections using the
Alternate-Protocol response header and for HTTPS connections using NPN.  In both
cases I think my HttpNetworkTransaction restarting during DNS and connect()
should apply.

WDYT?  If you review this CL and like where it's going I'll add tests.

[mmenke, 2014-02-06 15:08:31]

On 2014/02/06 03:49:04, pauljensen wrote:
> On 2014/01/30 03:46:20, pauljensen wrote:
> > On 2014/01/28 17:58:15, mmenke wrote:
> > > On 2014/01/28 17:48:18, pauljensen wrote:
> > > > Matt, What do you think of the general idea here?  If you like the idea
> I'll
> > > > work to finish off the changes by adding tests.
> > > 
> > > So this should catch for the socket pool ERR_NETWORK_CHANGED and the host
> > > resolver one.
> > > 
> > > Potential problem:  Is it possible for us to get, say, one from the
> > > HostResolver, start a new request, and then get another error from the
> socket
> > > pool?  I'm assuming you've tested enough to know that doesn't happen, but
> > > relying on there being an async event somewhere in there so that doesn't
> > happen
> > > seems regression prone to me.
> > Good point, I've added a PostTask() delay in there so all network observers
> can
> > be flushed from the task queue; this is what I recommended for the
URLFetcher
> > restarting on ERR_NETWORK_CHANGED also.
> > 
> > > 
> > > Both the SocketPools and the HostResolver are complicated enough that I
> think
> > > that retrying in the HttpNetworkTransaction makes sense.
> > > 
> > > Are there any SPDY/QUIC cases this can catch?  They presumably mostly
cause
> > the
> > > error after streams have been created, but I'd still like to know.
> > I think this could catch initial SpdySession establishment DNS and connect()
> > failures.  For QUIC I imagine this could catch ERR_NETWORK_CHANGED during
DNS,
> > I'll have to investigate what QUIC does for network changes during
> "connecting".
> >  FYI I'm working to reduce ERR_NETWORK_CHANGED from Spdy here:
> > http://crrev.com/129873010
> 
> I did some more investigation/thinking about the SPDY and QUIC cases.  Taking
> into account my http://crrev.com/129873010 change, we really shouldn't be
> getting ERR_NETWORK_CHANGED from requests using SPDY (or QUIC if similar
changes
> can be implemented) once the SpdySession is established.  So the only
> ERR_NETWORK_CHANGED errors we do care about will surface during SpdySession
> establishment.  I believe SPDY (and hence the choice of establishing a
> SpdySession) is selected in one of two ways:  for HTTP connections using the
> Alternate-Protocol response header and for HTTPS connections using NPN.  In
both
> cases I think my HttpNetworkTransaction restarting during DNS and connect()
> should apply.
> 
> WDYT?  If you review this CL and like where it's going I'll add tests.

Apologies for slowness, I'm firefighting this week.  I'll get to this when I
have time.  I'm sufficiently paranoid that I'd rather get this in after branch,
anyways.

[mmenke, 2014-02-13 17:24:09]

On 2014/02/06 15:08:31, mmenke wrote:
> On 2014/02/06 03:49:04, pauljensen wrote:
> > On 2014/01/30 03:46:20, pauljensen wrote:
> > > On 2014/01/28 17:58:15, mmenke wrote:
> > > > On 2014/01/28 17:48:18, pauljensen wrote:
> > > > > Matt, What do you think of the general idea here?  If you like the
idea
> > I'll
> > > > > work to finish off the changes by adding tests.
> > > > 
> > > > So this should catch for the socket pool ERR_NETWORK_CHANGED and the
host
> > > > resolver one.
> > > > 
> > > > Potential problem:  Is it possible for us to get, say, one from the
> > > > HostResolver, start a new request, and then get another error from the
> > socket
> > > > pool?  I'm assuming you've tested enough to know that doesn't happen,
but
> > > > relying on there being an async event somewhere in there so that doesn't
> > > happen
> > > > seems regression prone to me.
> > > Good point, I've added a PostTask() delay in there so all network
observers
> > can
> > > be flushed from the task queue; this is what I recommended for the
> URLFetcher
> > > restarting on ERR_NETWORK_CHANGED also.
> > > 
> > > > 
> > > > Both the SocketPools and the HostResolver are complicated enough that I
> > think
> > > > that retrying in the HttpNetworkTransaction makes sense.
> > > > 
> > > > Are there any SPDY/QUIC cases this can catch?  They presumably mostly
> cause
> > > the
> > > > error after streams have been created, but I'd still like to know.
> > > I think this could catch initial SpdySession establishment DNS and
connect()
> > > failures.  For QUIC I imagine this could catch ERR_NETWORK_CHANGED during
> DNS,
> > > I'll have to investigate what QUIC does for network changes during
> > "connecting".
> > >  FYI I'm working to reduce ERR_NETWORK_CHANGED from Spdy here:
> > > http://crrev.com/129873010
> > 
> > I did some more investigation/thinking about the SPDY and QUIC cases. 
Taking
> > into account my http://crrev.com/129873010 change, we really shouldn't be
> > getting ERR_NETWORK_CHANGED from requests using SPDY (or QUIC if similar
> changes
> > can be implemented) once the SpdySession is established.  So the only
> > ERR_NETWORK_CHANGED errors we do care about will surface during SpdySession
> > establishment.  I believe SPDY (and hence the choice of establishing a
> > SpdySession) is selected in one of two ways:  for HTTP connections using the
> > Alternate-Protocol response header and for HTTPS connections using NPN.  In
> both
> > cases I think my HttpNetworkTransaction restarting during DNS and connect()
> > should apply.
> > 
> > WDYT?  If you review this CL and like where it's going I'll add tests.
> 
> Apologies for slowness, I'm firefighting this week.  I'll get to this when I
> have time.  I'm sufficiently paranoid that I'd rather get this in after
branch,
> anyways.

Forgot about this, I'll get back to this today.

[mmenke, 2014-02-13 20:13:03]

So sorry for slowness...  Was completely focused on two M34 issues, and kind of
dropped the ball on a lot of other things as a result.  Feel free to ping me
when I'm so slow.

I think this looks pretty good.

Some concerns:
* Proxy AUTH, particularly over non-HTTP proxies.  Some forms of auth require we
reuse a connection, so if we go:  Request->Proxy AUTH challenge->Request on same
socket, with auth -> Network changed, I'm not sure what will happen.  Setting up
proxy connections, including AUTH data, generally falls under CreateStream,
except for the HTTP over HTTP/SPDY proxy case, I believe.

* 4 retries seems like a lot to me.  You'll often see a flurry of network
changes at once, of course, but by that point, we'll probably have already
returned ERR_OFFLINE.  We may want to think of some way to handle that (If a
network change to offline, wait half a second for a network change before
restart?  Or just add in an arbitrary short wait?  I don't know, may want to
think about later, after recording some stats).

* Another problem with 4 retries is that makes the worse possible connection
timeout 4 times the previous worst possible connection timeout (Modulo DNS
caching, I suppose).  May want some logic to prevent that, or could try to get
some stats about it.

* I wonder about making it a default behavior of the network stack.  Admittedly,
we make sure we do it safely, and in practice, we probably want the behavior for
everything, it just makes me a little nervous.

[mmenke, 2014-02-13 20:20:16]

Some tiny nits.

https://codereview.chromium.org/137493008/diff/130001/net/url_request/url_fet...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/137493008/diff/130001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:526: if (max_retries_on_network_changes_)
I think it's weird to have two ways to do this.  Admittedly, this implementation
will re-send requests, even rePOSTing if needed, but I'm not a huge fan of
having two mechanisms to do this.  May not have a choice, though.  Should look
at callsites, I think.

https://codereview.chromium.org/137493008/diff/130001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/137493008/diff/130001/net/url_request/url_req...
net/url_request/url_request.cc:51: const int
kDefaultMaxAutomaticRetriesOnNetworkChanges = 3;
Think it's a bit confusing to have max retries, instead of max tries.  I think
it's ambiguous if the first try a "retry" or not?  Your comment here makes it
clear, but best not to have to duplicate it everywhere.

https://codereview.chromium.org/137493008/diff/130001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/137493008/diff/130001/net/url_request/url_req...
net/url_request/url_request.h:692: int
max_automatic_retries_on_network_changes() const {
Don't think we need an accessor, for now.  Not a huge deal, just prefer to
minimize public APIs until we know they're needed.

[mmenke, 2014-11-20 19:38:28]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org