Early connection migration in QUIC.

QUIC currently migrates connections on platform notifications. This CL implements an early connection migration, where QUIC aggressively tries to migrate QUIC connections to an alternate network when the current network starts to fail. Network failure is defined as a number of consecutive retransmission timeouts, currently 2.

Early migration is expected be useful specifically when an Android user moves from using a WiFi network  to a cellular network. Android  favors WiFi over cellular connectivity, even poor WiFi is preferred over good cellular connectivity. Commonly, as a user moves out of WiFi range, this preferential behavior causes the application to experience a (possibly extended) period of poor or no connectivity, causing the "parking-lot problem". Early migration should alleviate this problem.

NOTE: QUIC does not change connectivity state by bringing network interfaces up or down. QUIC only migrates connections to existing alternative interfaces.

BUG=576998
Committed: https://crrev.com/d36ada63f49d89a7a4c4ba91f8618654a7cf1313
Cr-Commit-Position: refs/heads/master@{#373997}

[Jana, 2016-01-21 01:51:44]

Description was changed from

==========
Aggressively attempts connection migration when sufficient retransmission
timeouts occur.

BUG=576998
==========

to

==========
Attempts early connection migration when sufficient retransmission timeouts
occur.

BUG=576998
==========

[Jana, 2016-01-21 02:43:24]

Description was changed from

==========
Attempts early connection migration when sufficient retransmission timeouts
occur.

BUG=576998
==========

to

==========
QUIC currently migrates connections on platform notifications. This CL
implements an early connection migration, where QUIC aggressively tries to
migrate QUIC connections to an alternate network when the current network starts
to fail. Network failure is defined as a number of consecutive retransmission
timeouts, currently 2.

Early migration is expected be useful specifically when an Android user moves
from using a WiFi network  to a cellular network. Android  favors WiFi over
cellular connectivity, even poor WiFi is preferred over good cellular
connectivity. Commonly, as a user moves out of WiFi range, this preferential
behavior causes the application to experience a (possibly extended) period of
poor or no connectivity, causing the "parking-lot problem". Early migration
should alleviate this problem.

NOTE: QUIC does not change connectivity state by bringing network interfaces up
or down. QUIC only migrates connections to existing alternative interfaces.

BUG=576998
==========

[Jana, 2016-02-02 01:59:12]

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

[Jana, 2016-02-02 01:59:13]

Ryan: Just a note that I'm doing a separate internal CL for the shared code
changes. Thanks!

[Ryan Hamilton, 2016-02-02 04:19:59]

https://codereview.chromium.org/1613513003/diff/60001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1613513003/diff/60001/net/http/http_network_s...
net/http/http_network_session.h:197: // the session experiences poor
connectivity.
There's a bit of singular/plural confusion here with "active QUIC sessions" vs
"the session".

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection.h:136: virtual void MaybeMigrateConnection() = 0;
Looks like this is basically shared code and should probably be landed in an
internal change first.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection.h:136: virtual void MaybeMigrateConnection() = 0;
I think OnPeerUnreachable() (or some such) would be a better name for this
method.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_sent_pack...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_sent_pack...
net/quic/quic_sent_packet_manager.cc:39: // Minimum number of consecutive RTOs
before .
Is this comment complete?

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:667: DCHECK(migrate_sessions_on_network_change_
|| !migrate_sessions_early_);
Can you add comments for these two blocks? These expressions are not immediately
understandable.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1244: void
QuicStreamFactory::MaybeMigrateSessionEarly(
Was this landed post-review, or did I miss it in the previous CL?

[Jana, 2016-02-02 06:53:53]

Thanks, Ryan. Happy to come back to this after the shared code lands, but
responses for now.

https://codereview.chromium.org/1613513003/diff/60001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1613513003/diff/60001/net/http/http_network_s...
net/http/http_network_session.h:197: // the session experiences poor
connectivity.
On 2016/02/02 04:19:59, Ryan Hamilton wrote:
> There's a bit of singular/plural confusion here with "active QUIC sessions" vs
> "the session".

Done.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_connection.h
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection.h:136: virtual void MaybeMigrateConnection() = 0;
On 2016/02/02 04:19:59, Ryan Hamilton wrote:
> I think OnPeerUnreachable() (or some such) would be a better name for this
> method.

Addressing in shared code.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_connectio...
net/quic/quic_connection.h:136: virtual void MaybeMigrateConnection() = 0;
On 2016/02/02 04:19:59, Ryan Hamilton wrote:
> Looks like this is basically shared code and should probably be landed in an
> internal change first.

Yes, I wanted to have this CL out for context.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_sent_pack...
File net/quic/quic_sent_packet_manager.cc (right):

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_sent_pack...
net/quic/quic_sent_packet_manager.cc:39: // Minimum number of consecutive RTOs
before .
On 2016/02/02 04:19:59, Ryan Hamilton wrote:
> Is this comment complete?

Obviously not :-) I'm fixing this in the shared code.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:667: DCHECK(migrate_sessions_on_network_change_
|| !migrate_sessions_early_);
On 2016/02/02 04:19:59, Ryan Hamilton wrote:
> Can you add comments for these two blocks? These expressions are not
immediately
> understandable.

Done.

https://codereview.chromium.org/1613513003/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1244: void
QuicStreamFactory::MaybeMigrateSessionEarly(
On 2016/02/02 04:19:59, Ryan Hamilton wrote:
> Was this landed post-review, or did I miss it in the previous CL?

This got added post-LGTM, and I am pretty sure it was due to some git magic that
I f***ed up. I most certainly didn't intend to add this code in, since it wasn't
getting used. Apologies.

[Ryan Hamilton, 2016-02-02 23:17:54]

Good progress! Please ping when you've rebased off of the internal CL.

[Jana, 2016-02-04 08:20:02]

On 2016/02/02 23:17:54, Ryan Hamilton wrote:
> Good progress! Please ping when you've rebased off of the internal CL.

Thanks, Ryan. Synced. PTAL.

[Ryan Hamilton, 2016-02-04 20:22:20]

Looks good, modulo a diff glitch?

https://codereview.chromium.org/1613513003/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection.h (right):

https://codereview.chromium.org/1613513003/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection.h:141: virtual void OnPathDegrading() = 0;
Isn't this part of the internal CL? Can you exclude the diffs from that CL here?

[Jana, 2016-02-05 00:48:14]

On 2016/02/04 20:22:20, Ryan Hamilton wrote:
> Looks good, modulo a diff glitch?
> 
>
https://codereview.chromium.org/1613513003/diff/120001/net/quic/quic_connecti...
> File net/quic/quic_connection.h (right):
> 
>
https://codereview.chromium.org/1613513003/diff/120001/net/quic/quic_connecti...
> net/quic/quic_connection.h:141: virtual void OnPathDegrading() = 0;
> Isn't this part of the internal CL? Can you exclude the diffs from that CL
here?

Thanks, Ryan. Rebased.

[Ryan Hamilton, 2016-02-05 01:32:56]

Sweet!

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1271: if (!migrate_sessions_early_ ||
(session->GetNumActiveStreams() == 0)) {
nit: no need for the extra ()s, I don't think.

Actually, why do the "no active streams check"? Shouldn't we close the session?
(Of course, if this code is triggered based off of RTOs, it seems quite likely
that there are open streams :>)

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1274: NetworkChangeNotifier::NetworkHandle
current_network =
You might consider "using NetworkChangeNotifier::NetworkHandle" up at the top of
this file so as to reduce some of the noise.

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1277: FindAlternateNetwork(current_network);
Might as well just inline this:

NetworkChangeNotifier::NetworkHandle new_network =
    FindAlternateNetwork(session->GetDefaultSocket()->GetBoundNetwork());

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:1970: 
Either here or below, could you test that the second session is closed?

[Jana, 2016-02-05 03:03:31]

Thanks Ryan! See below.

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1271: if (!migrate_sessions_early_ ||
(session->GetNumActiveStreams() == 0)) {
On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> nit: no need for the extra ()s, I don't think.

Yeah, I put those for clarity, but maybe it's more clutter. Done.

> Actually, why do the "no active streams check"? Shouldn't we close the
session?
> (Of course, if this code is triggered based off of RTOs, it seems quite likely
> that there are open streams :>)

I think it makes sense to migrate sessions aggressively, but I don't see a
benefit in aggressively closing sessions. The platform signal
(OnNetworkSoonToDisconnect or OnNetworkDisconnected) will handle closing these
connections. At any rate, as you point out, this should not really happen since
this code is currently triggered by RTOs. (... of course, we can't make that
assumption here since the QuicStreamFactory is unaware of how this method is
getting called.)

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1274: NetworkChangeNotifier::NetworkHandle
current_network =
On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> You might consider "using NetworkChangeNotifier::NetworkHandle" up at the top
of
> this file so as to reduce some of the noise.

Yeah, I wanted that too :-)  NetworkHandle is a  typedef inside
NetworkChangeNotifier, so this doesn't work.

I thought about this some more, and I've added a typedef up above within the
file's namespace that makes this better I think... wdyt? I'm not sure if this is
a good idea, so I'll take your lead here.

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1277: FindAlternateNetwork(current_network);
On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> Might as well just inline this:
> 
> NetworkChangeNotifier::NetworkHandle new_network =
>     FindAlternateNetwork(session->GetDefaultSocket()->GetBoundNetwork());

Done.

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:1970: 
On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> Either here or below, could you test that the second session is closed?

Done. Also added this check in an earlier connection migration test.

[Ryan Hamilton, 2016-02-05 17:57:38]

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1271: if (!migrate_sessions_early_ ||
(session->GetNumActiveStreams() == 0)) {
On 2016/02/05 03:03:31, Jana wrote:
> On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> > Actually, why do the "no active streams check"? Shouldn't we close the
> session?
> > (Of course, if this code is triggered based off of RTOs, it seems quite
likely
> > that there are open streams :>)
> 
> I think it makes sense to migrate sessions aggressively, but I don't see a
> benefit in aggressively closing sessions. The platform signal
> (OnNetworkSoonToDisconnect or OnNetworkDisconnected) will handle closing these
> connections. At any rate, as you point out, this should not really happen
since
> this code is currently triggered by RTOs. (... of course, we can't make that
> assumption here since the QuicStreamFactory is unaware of how this method is
> getting called.)

I guess I'm confused about the motivations for early migration. If we have a
session which is idle and not working (because RTOs are firing) why would we
want to keep it around to use (poorly?) for future requests? Particularly when a
similar session which is not idle would become unavailable for future requests?

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1274: NetworkChangeNotifier::NetworkHandle
current_network =
On 2016/02/05 03:03:31, Jana wrote:
> On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> > You might consider "using NetworkChangeNotifier::NetworkHandle" up at the
top
> of
> > this file so as to reduce some of the noise.
> 
> Yeah, I wanted that too :-)  NetworkHandle is a  typedef inside
> NetworkChangeNotifier, so this doesn't work.

Really? f'n C++. What's the error?

> I thought about this some more, and I've added a typedef up above within the
> file's namespace that makes this better I think... wdyt? I'm not sure if this
is
> a good idea, so I'll take your lead here.

I think there's a new C++11 hotness that might be slightly preferable. I'm not
sure why using is better than typedef, but people seem to like it.

using NetworkHandle = NetworkChangeNotifier::NetworkHandle;

[Jana, 2016-02-05 22:24:36]

Hm. I thought I sent this out earlier, but I clearly didn't.

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1271: if (!migrate_sessions_early_ ||
(session->GetNumActiveStreams() == 0)) {
On 2016/02/05 17:57:37, Ryan Hamilton wrote:
> On 2016/02/05 03:03:31, Jana wrote:
> > On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> > > Actually, why do the "no active streams check"? Shouldn't we close the
> > session?
> > > (Of course, if this code is triggered based off of RTOs, it seems quite
> likely
> > > that there are open streams :>)
> > 
> > I think it makes sense to migrate sessions aggressively, but I don't see a
> > benefit in aggressively closing sessions. The platform signal
> > (OnNetworkSoonToDisconnect or OnNetworkDisconnected) will handle closing
these
> > connections. At any rate, as you point out, this should not really happen
> since
> > this code is currently triggered by RTOs. (... of course, we can't make that
> > assumption here since the QuicStreamFactory is unaware of how this method is
> > getting called.)
> 
> I guess I'm confused about the motivations for early migration. If we have a
> session which is idle and not working (because RTOs are firing) why would we
> want to keep it around to use (poorly?) for future requests? Particularly when
a
> similar session which is not idle would become unavailable for future
requests?

Per our offline conversation, we'll migrate all sessions that have either active
streams or unacked data; so I'm removing the active streams check here.

https://codereview.chromium.org/1613513003/diff/140001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1274: NetworkChangeNotifier::NetworkHandle
current_network =
On 2016/02/05 17:57:37, Ryan Hamilton wrote:
> On 2016/02/05 03:03:31, Jana wrote:
> > On 2016/02/05 01:32:55, Ryan Hamilton wrote:
> > > You might consider "using NetworkChangeNotifier::NetworkHandle" up at the
> top
> > of
> > > this file so as to reduce some of the noise.
> > 
> > Yeah, I wanted that too :-)  NetworkHandle is a  typedef inside
> > NetworkChangeNotifier, so this doesn't work.
> 
> Really? f'n C++. What's the error?

"using declaration cannot refer to class member".

 
> > I thought about this some more, and I've added a typedef up above within the
> > file's namespace that makes this better I think... wdyt? I'm not sure if
this
> is
> > a good idea, so I'll take your lead here.
> 
> I think there's a new C++11 hotness that might be slightly preferable. I'm not
> sure why using is better than typedef, but people seem to like it.
> 
> using NetworkHandle = NetworkChangeNotifier::NetworkHandle;

Ah, cool. This works, so I've made it so.

[Ryan Hamilton, 2016-02-05 22:39:59]

lgtm

[Jana, 2016-02-06 00:59:22]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2016-02-06 01:00:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1613513003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1613513003/180001

[commit-bot: I haz the power, 2016-02-06 02:42:14]

Description was changed from

==========
QUIC currently migrates connections on platform notifications. This CL
implements an early connection migration, where QUIC aggressively tries to
migrate QUIC connections to an alternate network when the current network starts
to fail. Network failure is defined as a number of consecutive retransmission
timeouts, currently 2.

Early migration is expected be useful specifically when an Android user moves
from using a WiFi network  to a cellular network. Android  favors WiFi over
cellular connectivity, even poor WiFi is preferred over good cellular
connectivity. Commonly, as a user moves out of WiFi range, this preferential
behavior causes the application to experience a (possibly extended) period of
poor or no connectivity, causing the "parking-lot problem". Early migration
should alleviate this problem.

NOTE: QUIC does not change connectivity state by bringing network interfaces up
or down. QUIC only migrates connections to existing alternative interfaces.

BUG=576998
==========

to

==========
QUIC currently migrates connections on platform notifications. This CL
implements an early connection migration, where QUIC aggressively tries to
migrate QUIC connections to an alternate network when the current network starts
to fail. Network failure is defined as a number of consecutive retransmission
timeouts, currently 2.

Early migration is expected be useful specifically when an Android user moves
from using a WiFi network  to a cellular network. Android  favors WiFi over
cellular connectivity, even poor WiFi is preferred over good cellular
connectivity. Commonly, as a user moves out of WiFi range, this preferential
behavior causes the application to experience a (possibly extended) period of
poor or no connectivity, causing the "parking-lot problem". Early migration
should alleviate this problem.

NOTE: QUIC does not change connectivity state by bringing network interfaces up
or down. QUIC only migrates connections to existing alternative interfaces.

BUG=576998
==========

[commit-bot: I haz the power, 2016-02-06 02:42:15]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-02-06 02:44:00]

Description was changed from

==========
QUIC currently migrates connections on platform notifications. This CL
implements an early connection migration, where QUIC aggressively tries to
migrate QUIC connections to an alternate network when the current network starts
to fail. Network failure is defined as a number of consecutive retransmission
timeouts, currently 2.

Early migration is expected be useful specifically when an Android user moves
from using a WiFi network  to a cellular network. Android  favors WiFi over
cellular connectivity, even poor WiFi is preferred over good cellular
connectivity. Commonly, as a user moves out of WiFi range, this preferential
behavior causes the application to experience a (possibly extended) period of
poor or no connectivity, causing the "parking-lot problem". Early migration
should alleviate this problem.

NOTE: QUIC does not change connectivity state by bringing network interfaces up
or down. QUIC only migrates connections to existing alternative interfaces.

BUG=576998
==========

to

==========
QUIC currently migrates connections on platform notifications. This CL
implements an early connection migration, where QUIC aggressively tries to
migrate QUIC connections to an alternate network when the current network starts
to fail. Network failure is defined as a number of consecutive retransmission
timeouts, currently 2.

Early migration is expected be useful specifically when an Android user moves
from using a WiFi network  to a cellular network. Android  favors WiFi over
cellular connectivity, even poor WiFi is preferred over good cellular
connectivity. Commonly, as a user moves out of WiFi range, this preferential
behavior causes the application to experience a (possibly extended) period of
poor or no connectivity, causing the "parking-lot problem". Early migration
should alleviate this problem.

NOTE: QUIC does not change connectivity state by bringing network interfaces up
or down. QUIC only migrates connections to existing alternative interfaces.

BUG=576998

Committed: https://crrev.com/d36ada63f49d89a7a4c4ba91f8618654a7cf1313
Cr-Commit-Position: refs/heads/master@{#373997}
==========

[commit-bot: I haz the power, 2016-02-06 02:44:01]

Patchset 10 (id:??) landed as
https://crrev.com/d36ada63f49d89a7a4c4ba91f8618654a7cf1313
Cr-Commit-Position: refs/heads/master@{#373997}