Early connection migration in QUIC.

net/quic/quic_stream_factory.cc

Index: net/quic/quic_stream_factory.cc
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index 703e9ea6cf0c4c1b6ddb4062db7949499f267a2a..1eb4b443d48458d5304ec30a85a1579e0e31abf4 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -591,6 +591,7 @@ QuicStreamFactory::QuicStreamFactory(
     bool disable_quic_on_timeout_with_open_streams,
     int idle_connection_timeout_seconds,
     bool migrate_sessions_on_network_change,
+    bool migrate_sessions_early,
     const QuicTagVector& connection_options)
     : require_confirmation_(true),
       host_resolver_(host_resolver),
@@ -640,6 +641,8 @@ QuicStreamFactory::QuicStreamFactory(
       migrate_sessions_on_network_change_(
           migrate_sessions_on_network_change &&
           NetworkChangeNotifier::AreNetworkHandlesSupported()),
+      migrate_sessions_early_(migrate_sessions_early &&
+                              migrate_sessions_on_network_change_),
       port_seed_(random_generator_->RandUint64()),
       check_persisted_supports_quic_(true),
       has_initialized_data_(false),
@@ -680,8 +683,12 @@ QuicStreamFactory::QuicStreamFactory(
         new PropertiesBasedQuicServerInfoFactory(http_server_properties_));
   }
 
-  DCHECK(
-      !(close_sessions_on_ip_change_ && migrate_sessions_on_network_change_));
+  // migrate_sessions_early_ should only be set to true if
+  // migrate_sessions_on_network_change_ is set to true.
+  DCHECK(migrate_sessions_on_network_change_ || !migrate_sessions_early_);
+  // close_sessions_on_ip_change_ and migrate_sessions_on_network_change_ should
+  // never be simultaneously set to true.
+  DCHECK(!close_sessions_on_ip_change_ || !migrate_sessions_on_network_change_);
   if (migrate_sessions_on_network_change_) {
     NetworkChangeNotifier::AddNetworkObserver(this);
   } else if (close_sessions_on_ip_change_) {
@@ -1261,7 +1268,7 @@ void QuicStreamFactory::MaybeMigrateOrCloseSessions(
 
 void QuicStreamFactory::MaybeMigrateSessionEarly(
     QuicChromiumClientSession* session) {
-  if (session->GetNumActiveStreams() == 0) {
+  if (!migrate_sessions_early_ || (session->GetNumActiveStreams() == 0)) {

[Ryan Hamilton, 2016/02/05 01:32:55]

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 :>)

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

Yeah, I put those for clarity, but maybe it's more clutter. Done.
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.)

[Ryan Hamilton, 2016/02/05 17:57:37]

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?

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

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.

     return;
   }
   NetworkChangeNotifier::NetworkHandle current_network =

[Ryan Hamilton, 2016/02/05 01:32:55]

You might consider "using NetworkChangeNotifier::NetworkHandle" up at the top of
this file so as to reduce some of the noise.

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

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.

[Ryan Hamilton, 2016/02/05 17:57:37]

Really? f'n C++. What's the error?
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]

"using declaration cannot refer to class member".

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