QUIC - disable QUIC under recent pathological connection errors.

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 02b0b986be890fff786eafe859914e1d963c3fa4..fd1281a4a78f4135621b0b01d8968233a0d046d9 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -565,6 +565,9 @@ QuicStreamFactory::QuicStreamFactory(
     bool prefer_aes,
     int max_number_of_lossy_connections,
     float packet_loss_threshold,
+    int max_epitaphs,
+    int threshold_public_resets_post_handshake,
+    int threshold_timeouts_streams_open,
     int socket_receive_buffer_size,
     const QuicTagVector& connection_options)
     : require_confirmation_(true),
@@ -591,6 +594,14 @@ QuicStreamFactory::QuicStreamFactory(
       prefer_aes_(prefer_aes),
       max_number_of_lossy_connections_(max_number_of_lossy_connections),
       packet_loss_threshold_(packet_loss_threshold),
+      max_epitaphs_(max_epitaphs),
+      public_resets_post_handshake_(0),
+      timeouts_streams_open_(0),
+      max_public_resets_post_handshake_(0),
+      max_timeouts_streams_open_(0),
+      threshold_timeouts_streams_open_(threshold_timeouts_streams_open),
+      threshold_public_resets_post_handshake_(
+          threshold_public_resets_post_handshake),
       socket_receive_buffer_size_(socket_receive_buffer_size),
       port_seed_(random_generator_->RandUint64()),
       check_persisted_supports_quic_(true),
@@ -620,6 +631,17 @@ QuicStreamFactory::QuicStreamFactory(
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
+  // TODO: need a separate max to track this.

[Ryan Hamilton, 2015/06/30 18:55:32]

TODO:(ckrasic) since you have the context?

[Buck, 2015/07/01 19:06:19]

Removed, the comment was obsolete.

+  if (max_public_resets_post_handshake_ > 0) {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        "Net.QuicStreamFactory.PublicResetsPostHandshake",
+        public_resets_post_handshake_, 0, max_epitaphs_, max_epitaphs_ / 2);
+  }
+  if (max_timeouts_streams_open_ > 0) {
+    UMA_HISTOGRAM_CUSTOM_COUNTS("Net.QuicStreamFactory.TimeoutsStreamsOpen",
+                                public_resets_post_handshake_, 0, max_epitaphs_,
+                                max_epitaphs_ / 2);
+  }

[Ryan Hamilton, 2015/06/30 18:55:32]

I think this is too late for histograms to be of much use. By the time the
network stack is being shut down, the histograms services is already dead. This
won't crash, but I don't think it'll end up logging anything :/  Perhaps log
when the total number of connections is > some value?

[Buck, 2015/07/01 19:06:19]

Ok.  I've moved these to where max_* get incremented.  So the histograms will be
cumulative now.  I think it will still give me what I need which is data guide
setting the thresholds.

   CloseAllSessions(ERR_ABORTED);
   while (!all_sessions_.empty()) {
     delete all_sessions_.begin()->first;
@@ -821,9 +843,25 @@ scoped_ptr<QuicHttpStream> QuicStreamFactory::CreateFromSession(
   return scoped_ptr<QuicHttpStream>(new QuicHttpStream(session->GetWeakPtr()));
 }
 
+QuicErrorCode QuicStreamFactory::QuicDisabledReason(uint16 port) {
+  if (max_number_of_lossy_connections_ > 0 &&
+      number_of_lossy_connections_[port] >= max_number_of_lossy_connections_) {
+    return QUIC_BAD_PACKET_LOSS_RATE;
+  }
+  if (threshold_public_resets_post_handshake_ > 0 &&
+      public_resets_post_handshake_ >=
+          threshold_public_resets_post_handshake_) {
+    return QUIC_PUBLIC_RESETS_POST_HANDSHAKE;
+  }
+  if (threshold_timeouts_streams_open_ > 0 &&
+      timeouts_streams_open_ >= threshold_timeouts_streams_open_) {
+    return QUIC_TIMEOUTS_STREAMS_OPEN;
+  }
+  return QUIC_NO_ERROR;
+}
+
 bool QuicStreamFactory::IsQuicDisabled(uint16 port) {
-  return max_number_of_lossy_connections_ > 0 &&
-         number_of_lossy_connections_[port] >= max_number_of_lossy_connections_;
+  return (QuicDisabledReason(port) != QUIC_NO_ERROR);

[Ryan Hamilton, 2015/06/30 18:55:32]

nit: no need for the extra ()s

[Buck, 2015/07/01 19:06:19]

Done.

 }
 
 bool QuicStreamFactory::OnHandshakeConfirmed(QuicClientSession* session,
@@ -854,17 +892,17 @@ bool QuicStreamFactory::OnHandshakeConfirmed(QuicClientSession* session,
                  max_number_of_lossy_connections_));
   }
 
-  bool is_quic_disabled = IsQuicDisabled(port);
-  if (is_quic_disabled) {
+  QuicErrorCode error_code = QuicDisabledReason(port);
+  if (error_code != QUIC_NO_ERROR) {
     // Close QUIC connection if Quic is disabled for this port.
-    session->CloseSessionOnErrorAndNotifyFactoryLater(
-        ERR_ABORTED, QUIC_BAD_PACKET_LOSS_RATE);
+    session->CloseSessionOnErrorAndNotifyFactoryLater(ERR_ABORTED, error_code);
 
     // If this bad packet loss rate disabled the QUIC, then record it.
-    if (!was_quic_disabled)
+    if (!was_quic_disabled && error_code == QUIC_BAD_PACKET_LOSS_RATE)
       UMA_HISTOGRAM_SPARSE_SLOWLY("Net.QuicStreamFactory.QuicIsDisabled", port);
+    return true;
   }
-  return is_quic_disabled;
+  return false;

[Ryan Hamilton, 2015/06/30 18:55:32]

Your call, but I might write this as:

if (error_code == NO_ERROR)
  return true;

// Close QUIC ...
..

(Just to remove a level of nesting now that we're using different explicit
return statements.

[Buck, 2015/07/01 19:06:19]

Done.

 }
 
 void QuicStreamFactory::OnIdleSession(QuicClientSession* session) {
@@ -897,8 +935,44 @@ void QuicStreamFactory::OnSessionGoingAway(QuicClientSession* session) {
   session_aliases_.erase(session);
 }
 
+void QuicStreamFactory::MaybeDisableQuic(QuicClientSession* session) {
+  DCHECK(session);
+  uint16 port = session->server_id().port();
+  if (IsQuicDisabled(port))
+    return;
+
+  // Expire the oldest epitaph if appropriate.  This enforces that we
+  // only consider the max_epitaphs_ most recent sessions.
+  QuicClientSessionEpitaph epitaph;
+  if ((int)epitaphs_.size() == max_epitaphs_) {

[Ryan Hamilton, 2015/06/30 18:55:32]

nit: static_cast

[Buck, 2015/07/01 19:06:19]

Done.

+    epitaph = epitaphs_.front();
+    epitaphs_.pop_front();
+    if (epitaph == QUIC_EPITAPH_PUBLIC_RESET_POST_HANDSHAKE) {
+      public_resets_post_handshake_--;
+    } else if (epitaph == QUIC_EPITAPH_TIMEOUT_STREAMS_OPEN) {
+      timeouts_streams_open_--;
+    }
+  }
+  epitaph = session->epitaph();
+  epitaphs_.push_back(epitaph);
+  if (epitaph == QUIC_EPITAPH_PUBLIC_RESET_POST_HANDSHAKE) {
+    public_resets_post_handshake_++;
+  } else if (epitaph == QUIC_EPITAPH_TIMEOUT_STREAMS_OPEN) {
+    timeouts_streams_open_++;
+  }
+  max_timeouts_streams_open_ =
+      std::max(timeouts_streams_open_, max_timeouts_streams_open_);
+  max_public_resets_post_handshake_ = std::max(
+      public_resets_post_handshake_, max_public_resets_post_handshake_);

[Ryan Hamilton, 2015/06/30 18:55:31]

I'm not following the logic of resetting max_ here. Can you add a comment? (I'm
sure it makes sense)

[Buck, 2015/07/01 19:06:19]

I changed this so it is hopefully more direct and understandable.

[Ryan Hamilton, 2015/07/06 18:25:41]

Oh, I see. I was confusing max_disabled_reasons_ with max_<particular_reason>_.
*facepalm*

+  if (IsQuicDisabled(port)) {
+    UMA_HISTOGRAM_ENUMERATION("Net.QuicStreamFactory.DisabledReasons", epitaph,
+                              QUIC_EPITAPH_MAX);
+  }
+}
+
 void QuicStreamFactory::OnSessionClosed(QuicClientSession* session) {
   DCHECK_EQ(0u, session->GetNumOpenStreams());
+  MaybeDisableQuic(session);
   OnSessionGoingAway(session);
   delete session;
   all_sessions_.erase(session);