Early connection migration in QUIC.

net/quic/quic_stream_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_stream_factory.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 573 matching lines @@
     int max_disabled_reasons,
     int threshold_public_resets_post_handshake,
     int threshold_timeouts_with_open_streams,
     int socket_receive_buffer_size,
     bool delay_tcp_race,
     int max_server_configs_stored_in_properties,
     bool close_sessions_on_ip_change,
     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),
       client_socket_factory_(client_socket_factory),
       http_server_properties_(http_server_properties),
       transport_security_state_(transport_security_state),
       cert_transparency_verifier_(cert_transparency_verifier),
       quic_crypto_client_stream_factory_(quic_crypto_client_stream_factory),
       random_generator_(random_generator),
       clock_(clock),
@@ skipping 29 matching lines @@
           threshold_public_resets_post_handshake),
       socket_receive_buffer_size_(socket_receive_buffer_size),
       delay_tcp_race_(delay_tcp_race),
       yield_after_packets_(kQuicYieldAfterPacketsRead),
       yield_after_duration_(QuicTime::Delta::FromMilliseconds(
           kQuicYieldAfterDurationMilliseconds)),
       close_sessions_on_ip_change_(close_sessions_on_ip_change),
       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),
       task_runner_(nullptr),
       weak_factory_(this) {
   if (disable_quic_on_timeout_with_open_streams)
     threshold_timeouts_with_open_streams_ = 1;
   DCHECK(transport_security_state_);
   DCHECK(http_server_properties_);
   crypto_config_.set_user_agent_id(user_agent_id);
@@ skipping 20 matching lines @@
   if (!IsEcdsaSupported())
     crypto_config_.DisableEcdsa();
   // When disk cache is used to store the server configs, HttpCache code calls
   // |set_quic_server_info_factory| if |quic_server_info_factory_| wasn't
   // created.
   if (max_server_configs_stored_in_properties > 0) {
     quic_server_info_factory_.reset(
         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_) {
     NetworkChangeNotifier::AddIPAddressObserver(this);
   }
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
   CloseAllSessions(ERR_ABORTED, QUIC_CONNECTION_CANCELLED);
   while (!all_sessions_.empty()) {
@@ skipping 559 matching lines @@
                                      QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
       }
       continue;
     }
     MigrateSessionToNetwork(session, new_network);
   }
 }
 
 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.

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

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

Might as well just inline this:

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

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

Done.

   if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
     // No alternate network found.
     return;
   }
   OnSessionGoingAway(session);
   MigrateSessionToNetwork(session, new_network);
 }
 
 void QuicStreamFactory::MigrateSessionToNetwork(
     QuicChromiumClientSession* session,
@@ skipping 373 matching lines @@
   // Since the session was active, there's no longer an
   // HttpStreamFactoryImpl::Job running which can mark it broken, unless the TCP
   // job also fails. So to avoid not using QUIC when we otherwise could, we mark
   // it as recently broken, which means that 0-RTT will be disabled but we'll
   // still race.
   http_server_properties_->MarkAlternativeServiceRecentlyBroken(
       alternative_service);
 }
 
 }  // namespace net