Migrates QUIC sessions to a new network when old network is (about to be) disconnected.

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 2443381e3e4f20654a2aba7b839876f455279a42..6caf3d57c1f19cf387ffda2de0c6dfcce4157ee7 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -70,6 +70,15 @@ enum CreateSessionFailure {
   CREATION_ERROR_MAX
 };
 
+enum QuicConnectionMigrationStatus {
+  MIGRATION_STATUS_NO_MIGRATABLE_STREAMS,
+  MIGRATION_STATUS_ALREADY_MIGRATED,
+  MIGRATION_STATUS_INTERNAL_ERROR,
+  MIGRATION_STATUS_TOO_MANY_CHANGES,
+  MIGRATION_STATUS_SUCCESS,
+  MIGRATION_STATUS_MAX
+};
+
 // The maximum receive window sizes for QUIC sessions and streams.
 const int32 kQuicSessionMaxRecvWindowSize = 15 * 1024 * 1024;  // 15 MB
 const int32 kQuicStreamMaxRecvWindowSize = 6 * 1024 * 1024;    // 6 MB
@@ -571,6 +580,7 @@ QuicStreamFactory::QuicStreamFactory(
     bool store_server_configs_in_properties,
     bool close_sessions_on_ip_change,
     int idle_connection_timeout_seconds,
+    bool migrate_sessions_on_network_change,
     const QuicTagVector& connection_options)
     : require_confirmation_(true),
       host_resolver_(host_resolver),
@@ -618,6 +628,7 @@ QuicStreamFactory::QuicStreamFactory(
           kQuicYieldAfterDurationMilliseconds)),
       store_server_configs_in_properties_(store_server_configs_in_properties),
       close_sessions_on_ip_change_(close_sessions_on_ip_change),
+      migrate_sessions_on_network_change_(migrate_sessions_on_network_change),

[Ryan Hamilton, 2015/12/18 21:51:23]

Presumably we don't want both close_ and migrate_ to be true at the same time?
We could use a tri-state enum for this. Or in the worst case DCHECK(!(close_ &&
migrate_));

[Jana, 2015/12/21 23:10:19]

Hmm -- yes, I think we'd want them to be mutually exclusive until we decide what
default behaviors we want. Given that there's mutual exclusion guaranteed below
by only registering for one of the two observers below, I'm happy with adding a
DCHECK here (done below). SG?

       port_seed_(random_generator_->RandUint64()),
       check_persisted_supports_quic_(true),
       has_initialized_data_(false),
@@ -656,7 +667,10 @@ QuicStreamFactory::QuicStreamFactory(
         new PropertiesBasedQuicServerInfoFactory(http_server_properties_));
   }
 
-  if (close_sessions_on_ip_change_) {
+  if (migrate_sessions_on_network_change_ &&
+      NetworkChangeNotifier::AreNetworkHandlesSupported()) {

[Ryan Hamilton, 2015/12/18 21:51:23]

This is basically Android only currently?

[Jana, 2015/12/21 23:10:19]

Yes, Android >= L

+    NetworkChangeNotifier::AddNetworkObserver(this);
+  } else if (close_sessions_on_ip_change_) {
     NetworkChangeNotifier::AddIPAddressObserver(this);
   }
 }
@@ -672,7 +686,10 @@ QuicStreamFactory::~QuicStreamFactory() {
     STLDeleteElements(&(active_jobs_[server_id]));
     active_jobs_.erase(server_id);
   }
-  if (close_sessions_on_ip_change_) {
+  if (migrate_sessions_on_network_change_ &&
+      NetworkChangeNotifier::AreNetworkHandlesSupported()) {
+    NetworkChangeNotifier::RemoveNetworkObserver(this);
+  } else if (close_sessions_on_ip_change_) {
     NetworkChangeNotifier::RemoveIPAddressObserver(this);
   }
 }
@@ -1099,16 +1116,19 @@ void QuicStreamFactory::CancelRequest(QuicStreamRequest* request) {
 }
 
 void QuicStreamFactory::CloseAllSessions(int error) {

[Ryan Hamilton, 2015/12/18 21:51:23]

I'd be inclined to remove this method and just use your new one instead.

[Jana, 2015/12/21 23:10:19]

Done.

+  CloseAllSessionsInner(error, QUIC_INTERNAL_ERROR);
+}
+
+void QuicStreamFactory::CloseAllSessionsInner(int error,
+                                              QuicErrorCode quic_error) {
   while (!active_sessions_.empty()) {
     size_t initial_size = active_sessions_.size();
-    active_sessions_.begin()->second->CloseSessionOnError(error,
-                                                          QUIC_INTERNAL_ERROR);
+    active_sessions_.begin()->second->CloseSessionOnError(error, quic_error);
     DCHECK_NE(initial_size, active_sessions_.size());
   }
   while (!all_sessions_.empty()) {
     size_t initial_size = all_sessions_.size();
-    all_sessions_.begin()->first->CloseSessionOnError(error,
-                                                      QUIC_INTERNAL_ERROR);
+    all_sessions_.begin()->first->CloseSessionOnError(error, quic_error);
     DCHECK_NE(initial_size, all_sessions_.size());
   }
   DCHECK(all_sessions_.empty());
@@ -1141,10 +1161,112 @@ void QuicStreamFactory::ClearCachedStatesInCryptoConfig() {
 }
 
 void QuicStreamFactory::OnIPAddressChanged() {
-  CloseAllSessions(ERR_NETWORK_CHANGED);
+  CloseAllSessionsInner(ERR_NETWORK_CHANGED, QUIC_IP_ADDRESS_CHANGED);
   set_require_confirmation(true);
 }
 
+void QuicStreamFactory::OnNetworkConnected(
+    NetworkChangeNotifier::NetworkHandle network) {}
+
+void QuicStreamFactory::OnNetworkMadeDefault(
+    NetworkChangeNotifier::NetworkHandle network) {}
+
+void QuicStreamFactory::OnNetworkDisconnected(
+    NetworkChangeNotifier::NetworkHandle network) {
+  MigrateOrCloseSessions(network);
+  set_require_confirmation(true);
+}
+
+void QuicStreamFactory::OnNetworkSoonToDisconnect(
+    NetworkChangeNotifier::NetworkHandle network) {
+  MigrateOrCloseSessions(network);
+}
+
+void QuicStreamFactory::MigrateOrCloseSessions(
+    NetworkChangeNotifier::NetworkHandle network) {
+  DCHECK_NE(NetworkChangeNotifier::kInvalidNetworkHandle, network);
+  // Find a new network that old sockets can be migrated to.

[Ryan Hamilton, 2015/12/18 21:51:23]

nit: s/sockets/connections/?

[Jana, 2015/12/21 23:10:19]

Done.

+  NetworkChangeNotifier::NetworkList network_list;
+  NetworkChangeNotifier::GetConnectedNetworks(&network_list);
+  NetworkChangeNotifier::NetworkHandle new_network =
+      NetworkChangeNotifier::kInvalidNetworkHandle;
+  for (NetworkChangeNotifier::NetworkHandle n : network_list) {
+    if (n == network) {
+      continue;
+    }
+    new_network = n;

[Ryan Hamilton, 2015/12/18 21:51:23]

Can you break; here?

for (...) {
  if (n != network) {
    new_newtwork = n;
    break;
  }
}

[Jana, 2015/12/21 23:10:19]

Done.

+  }
+
+  if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
+    CloseAllSessionsInner(ERR_NETWORK_CHANGED,
+                          QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);

[Ryan Hamilton, 2015/12/18 21:51:23]

It seems sad to do this in the "SoonToDisconnect" case, doesn't it?

[Jana, 2015/12/21 23:10:19]

Yes, it is a bit aggressive. I had two reasons for doing the mgiration here:
(i) This gives us some "drain time" to drain the old network., since the client
continues reading from the old socket. The client does not have another signal
later before "Disconnected", when migration may be not as seamless, since any
packets still inflight to the client from the server over the old network will
have to be retransmitted by the server over the new network, possibly after a
TLP period.
(ii) This signal is delivered when we're moving from Cellular to WiFi, and I
hypothesized that the sooner we migrate to WiFi, the better.

Do you have any other suggestions?

+    return;
+  }
+
+  QuicStreamFactory::SessionIdMap::iterator it = all_sessions_.begin();
+  while (it != all_sessions_.end()) {
+    QuicChromiumClientSession* session = it->first;
+    QuicServerId server_id = it->second;
+    ++it;
+
+    if (session->GetNumActiveStreams() == 0) {

[Ryan Hamilton, 2015/12/18 21:51:23]

Ah, that's why you were needing this method.

[Jana, 2015/12/21 23:10:19]

Acknowledged.

+      // Close idle sessions.
+      session->CloseSessionOnError(
+          ERR_NETWORK_CHANGED, QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS);
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSession.ConnectionMigration",
+                                MIGRATION_STATUS_NO_MIGRATABLE_STREAMS,
+                                MIGRATION_STATUS_MAX);

[Ryan Hamilton, 2015/12/18 21:51:23]

Please factor this out into a helper method which simply takes a single enum as
an argument.

[Jana, 2015/12/21 23:10:19]

I thought of doing this too simply to reduce clutter, but it seemed an overkill
(3 args vs. 1 arg). If you have the same reaction, I'll do it :-)

[Jana, 2015/12/21 23:10:19]

Done.

+      continue;
+    } else if (session->GetDefaultSocket()->GetBoundNetwork() == new_network) {
+      // If session is already using |network|, move on.
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSession.ConnectionMigration",
+                                MIGRATION_STATUS_ALREADY_MIGRATED,
+                                MIGRATION_STATUS_MAX);
+      continue;
+    }
+
+    // If session has active streams, (i) make the session GO_AWAY,
+    // (ii) create a new socket bound to the new network, and (iii)
+    // migrate the session to using the new socket.
+    OnSessionGoingAway(session);
+    QuicConnection* connection = session->connection();
+
+    // Use OS-specified port for socket (DEFAULT_BIND) since the connection is
+    // being migrated and not being newly created.

[Ryan Hamilton, 2015/12/18 21:51:23]

I don't understand this comment.

[Jana, 2015/12/21 23:10:19]

The point of the PortSuggester, AIUI, is to use a port for a new 0RTT connect so
that it maps to the same GFE as previously, to possibly increase 0RTT chances.
Given that the IP is changing, retaining the same port as earlier shouldn't get
us anything... right?

Modifying the comment per our conversation.

+    scoped_ptr<DatagramClientSocket> socket(
+        client_socket_factory_->CreateDatagramClientSocket(
+            DatagramSocket::DEFAULT_BIND, RandIntCallback(),
+            session->net_log().net_log(), session->net_log().source()));
+
+    if (ConfigureDatagramClientSocket(socket.get(), connection->peer_address(),
+                                      network) != OK) {
+      session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSession.ConnectionMigration",
+                                MIGRATION_STATUS_INTERNAL_ERROR,
+                                MIGRATION_STATUS_MAX);
+      continue;
+    }
+    scoped_ptr<QuicPacketReader> new_reader(new QuicPacketReader(
+        socket.get(), clock_.get(), session, yield_after_packets_,
+        yield_after_duration_, session->net_log()));
+    DefaultPacketWriterFactory packet_writer_factory(socket.get());
+    scoped_ptr<QuicPacketWriter> new_writer(
+        packet_writer_factory.Create(connection));
+
+    if (!session->MigrateToSocket(socket.Pass(), new_reader.Pass(),
+                                  new_writer.Pass())) {
+      session->CloseSessionOnError(ERR_NETWORK_CHANGED,
+                                   QUIC_CONNECTION_MIGRATION_TOO_MANY_CHANGES);
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSession.ConnectionMigration",
+                                MIGRATION_STATUS_TOO_MANY_CHANGES,
+                                MIGRATION_STATUS_MAX);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSession.ConnectionMigration",
+                                MIGRATION_STATUS_SUCCESS, MIGRATION_STATUS_MAX);
+    }
+  }
+}
+
 void QuicStreamFactory::OnSSLConfigChanged() {
   CloseAllSessions(ERR_CERT_DATABASE_CHANGED);
 }
@@ -1178,36 +1300,10 @@ bool QuicStreamFactory::HasActiveJob(const QuicServerId& key) const {
   return ContainsKey(active_jobs_, key);
 }
 
-int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
-                                     int cert_verify_flags,
-                                     scoped_ptr<QuicServerInfo> server_info,
-                                     const AddressList& address_list,
-                                     base::TimeTicks dns_resolution_end_time,
-                                     const BoundNetLog& net_log,
-                                     QuicChromiumClientSession** session) {
-  bool enable_port_selection = enable_port_selection_;
-  if (enable_port_selection &&
-      ContainsKey(gone_away_aliases_, server_id)) {
-    // Disable port selection when the server is going away.
-    // There is no point in trying to return to the same server, if
-    // that server is no longer handling requests.
-    enable_port_selection = false;
-    gone_away_aliases_.erase(server_id);
-  }
-
-  QuicConnectionId connection_id = random_generator_->RandUint64();
-  IPEndPoint addr = *address_list.begin();
-  scoped_refptr<PortSuggester> port_suggester =
-      new PortSuggester(server_id.host_port_pair(), port_seed_);
-  DatagramSocket::BindType bind_type = enable_port_selection ?
-      DatagramSocket::RANDOM_BIND :  // Use our callback.
-      DatagramSocket::DEFAULT_BIND;  // Use OS to randomize.
-  scoped_ptr<DatagramClientSocket> socket(
-      client_socket_factory_->CreateDatagramClientSocket(
-          bind_type,
-          base::Bind(&PortSuggester::SuggestPort, port_suggester),
-          net_log.net_log(), net_log.source()));
-
+int QuicStreamFactory::ConfigureDatagramClientSocket(
+    DatagramClientSocket* socket,
+    IPEndPoint addr,
+    NetworkChangeNotifier::NetworkHandle network) {
   if (enable_non_blocking_io_ &&
       client_socket_factory_ == ClientSocketFactory::GetDefaultFactory()) {
 #if defined(OS_WIN)
@@ -1215,25 +1311,29 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
 #endif
   }
 
-  int rv = socket->Connect(addr);
+  // If caller leaves network unspecified, use current default.
+  int rv;
+  if (network == NetworkChangeNotifier::kInvalidNetworkHandle) {
+    rv = socket->BindToDefaultNetwork();

[Ryan Hamilton, 2015/12/18 21:51:23]

Does this represent a behavior change? Is "BindToDefault" different from not
doing BindToDefault()?

[Jana, 2015/12/21 23:10:19]

Yes. This is a behavior change in that there was no explicit or implicit
bind()ing before.

[Ryan Hamilton, 2015/12/21 23:18:19]

I guess it probably doesn't matter, but I wonder if we want to "bind to default"
when we're not doing migration? Thinking about non-Android... Maybe it doesn't
matter?

+  } else {
+    rv = socket->BindToNetwork(network);
+  }
+  if (rv != OK) {
+    return rv;
+  }

[Ryan Hamilton, 2015/12/18 21:51:23]

nit: no {}s

[Jana, 2015/12/21 23:10:19]

I can never keep this together in my mind. Done.

 
+  rv = socket->Connect(addr);
   if (rv != OK) {
     HistogramCreateSessionFailure(CREATION_ERROR_CONNECTING_SOCKET);
     return rv;
   }
-  UMA_HISTOGRAM_COUNTS("Net.QuicEphemeralPortsSuggested",
-                       port_suggester->call_count());
-  if (enable_port_selection) {
-    DCHECK_LE(1u, port_suggester->call_count());
-  } else {
-    DCHECK_EQ(0u, port_suggester->call_count());
-  }
 
   rv = socket->SetReceiveBufferSize(socket_receive_buffer_size_);
   if (rv != OK) {
     HistogramCreateSessionFailure(CREATION_ERROR_SETTING_RECEIVE_BUFFER);
     return rv;
   }
+
   // Set a buffer large enough to contain the initial CWND's worth of packet
   // to work around the problem with CHLO packets being sent out with the
   // wrong encryption level, when the send buffer is full.
@@ -1253,14 +1353,60 @@ int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
     }
   }
 
-  DefaultPacketWriterFactory packet_writer_factory(socket.get());
+  return OK;
+}
+
+int QuicStreamFactory::CreateSession(const QuicServerId& server_id,
+                                     int cert_verify_flags,
+                                     scoped_ptr<QuicServerInfo> server_info,
+                                     const AddressList& address_list,
+                                     base::TimeTicks dns_resolution_end_time,
+                                     const BoundNetLog& net_log,
+                                     QuicChromiumClientSession** session) {
+  IPEndPoint addr = *address_list.begin();
+  bool enable_port_selection = enable_port_selection_;
+  if (enable_port_selection && ContainsKey(gone_away_aliases_, server_id)) {
+    // Disable port selection when the server is going away.
+    // There is no point in trying to return to the same server, if
+    // that server is no longer handling requests.
+    enable_port_selection = false;
+    gone_away_aliases_.erase(server_id);
+  }
+  scoped_refptr<PortSuggester> port_suggester =
+      new PortSuggester(server_id.host_port_pair(), port_seed_);
+  DatagramSocket::BindType bind_type =
+      enable_port_selection ? DatagramSocket::RANDOM_BIND
+                            :            // Use our callback.
+          DatagramSocket::DEFAULT_BIND;  // Use OS to randomize.
+
+  scoped_ptr<DatagramClientSocket> socket(
+      client_socket_factory_->CreateDatagramClientSocket(
+          bind_type, base::Bind(&PortSuggester::SuggestPort, port_suggester),
+          net_log.net_log(), net_log.source()));
+
+  // Passing in kInvalidNetworkHandle binds socket to default network.
+  int rv = ConfigureDatagramClientSocket(
+      socket.get(), addr, NetworkChangeNotifier::kInvalidNetworkHandle);
+  if (rv != OK) {
+    return rv;
+  }
 
+  UMA_HISTOGRAM_COUNTS("Net.QuicEphemeralPortsSuggested",
+                       port_suggester->call_count());
+  if (enable_port_selection) {
+    DCHECK_LE(1u, port_suggester->call_count());
+  } else {
+    DCHECK_EQ(0u, port_suggester->call_count());
+  }
+
+  DefaultPacketWriterFactory packet_writer_factory(socket.get());
   if (!helper_.get()) {
     helper_.reset(
         new QuicConnectionHelper(base::ThreadTaskRunnerHandle::Get().get(),
                                  clock_.get(), random_generator_));
   }
 
+  QuicConnectionId connection_id = random_generator_->RandUint64();
   QuicConnection* connection = new QuicConnection(
       connection_id, addr, helper_.get(), packet_writer_factory,
       true /* owns_writer */, Perspective::IS_CLIENT, supported_versions_);