To mitigate the effects of hanging 0-RTT QUIC connections,

To mitigate the effects of hanging 0-RTT QUIC connections,
set up a timer to cancel any requests, if the handshake takes too long.
The requests will be retried and will use either QUIC or TCP, whichever
connects first.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275552

[ramant (doing other things), 2014-06-05 22:27:19]

lgtm

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:453: base::TimeDelta::FromMilliseconds(300));
overly nit: wondering if 300ms is long enough for countries with high latency
(not high quality network connectivity).

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:211:
was_alternate_protocol_recently_broken_(false), // unused
nit: consider adding extra space before // unused

    "(false),  // unused"

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.h:128: // Caleed by a session whose connection has
timed out.
nit: "Caleed" -> "Called"?

[ramant (doing other things), 2014-06-05 22:33:47]

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:453: base::TimeDelta::FromMilliseconds(300));
On 2014/06/05 22:27:18, ramant wrote:
> overly nit: wondering if 300ms is long enough for countries with high latency
> (not high quality network connectivity).

I like this change. Should we do a similar change (timeout) if we couldn't load
server config from disk within 300ms?

thanks
raman (with 2 cents)

[Ryan Hamilton, 2014-06-05 22:47:51]

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:453: base::TimeDelta::FromMilliseconds(300));
On 2014/06/05 22:27:18, ramant wrote:
> overly nit: wondering if 300ms is long enough for countries with high latency
> (not high quality network connectivity).

It's a good question! :> The short answer is that we don't know what the "right"
value is, but since we give ipv6 a 300ms headstart, this seems like a sensible
first estimate. Keep in mind, if the handshake completes at 301ms, the
HttpNetworkTransaction will be busily retrying so it should complete almost
immediately.

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:453: base::TimeDelta::FromMilliseconds(300));
On 2014/06/05 22:33:47, ramant wrote:
> On 2014/06/05 22:27:18, ramant wrote:
> > overly nit: wondering if 300ms is long enough for countries with high
latency
> > (not high quality network connectivity).
> 
> I like this change. Should we do a similar change (timeout) if we couldn't
load
> server config from disk within 300ms?
> 
> thanks
> raman (with 2 cents)

Possibly. Of course, we're reading the configs from a *local* disk so it
shouldn't take *too* long. In fact, I think you had a histogram for this which
suggested that they were often very fast.

[Ryan Hamilton, 2014-06-05 22:48:10]

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:211:
was_alternate_protocol_recently_broken_(false), // unused
On 2014/06/05 22:27:18, ramant wrote:
> nit: consider adding extra space before // unused
> 
>     "(false),  // unused"

Done.

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory.h
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/318993004/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.h:128: // Caleed by a session whose connection has
timed out.
On 2014/06/05 22:27:19, ramant wrote:
> nit: "Caleed" -> "Called"?

Done.

[Ryan Hamilton, 2014-06-06 17:16:53]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:18:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/318993004/20001

[commit-bot: I haz the power, 2014-06-06 23:37:08]

Change committed as 275552