Sent QUIC "PING" frames when a stream is open and the connection

net/quic/quic_connection.cc

Index: net/quic/quic_connection.cc
diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc
index 99d841f03bb62e33e584b700de994692756d4403..84aef1c9b184279ae5618e4553181d38b9e2ff24 100644
--- a/net/quic/quic_connection.cc
+++ b/net/quic/quic_connection.cc
@@ -75,6 +75,8 @@ class AckAlarm : public QuicAlarm::Delegate {
 
  private:
   QuicConnection* connection_;
+
+  DISALLOW_COPY_AND_ASSIGN(AckAlarm);
 };
 
 // This alarm will be scheduled any time a data-bearing packet is sent out.
@@ -93,6 +95,8 @@ class RetransmissionAlarm : public QuicAlarm::Delegate {
 
  private:
   QuicConnection* connection_;
+
+  DISALLOW_COPY_AND_ASSIGN(RetransmissionAlarm);
 };
 
 // An alarm that is scheduled when the sent scheduler requires a
@@ -111,6 +115,8 @@ class SendAlarm : public QuicAlarm::Delegate {
 
  private:
   QuicConnection* connection_;
+
+  DISALLOW_COPY_AND_ASSIGN(SendAlarm);
 };
 
 class TimeoutAlarm : public QuicAlarm::Delegate {
@@ -127,6 +133,23 @@ class TimeoutAlarm : public QuicAlarm::Delegate {
 
  private:
   QuicConnection* connection_;
+
+  DISALLOW_COPY_AND_ASSIGN(TimeoutAlarm);
+};
+
+class PingAlarm : public QuicAlarm::Delegate {
+ public:
+  explicit PingAlarm(QuicConnection* connection)
+      : connection_(connection) {
+  }
+
+  virtual QuicTime OnAlarm() override {
+    connection_->SendPing();
+    return QuicTime::Zero();
+  }
+
+ private:
+  QuicConnection* connection_;
 };
 
 QuicConnection::PacketType GetPacketType(
@@ -191,6 +214,7 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id,
       send_alarm_(helper->CreateAlarm(new SendAlarm(this))),
       resume_writes_alarm_(helper->CreateAlarm(new SendAlarm(this))),
       timeout_alarm_(helper->CreateAlarm(new TimeoutAlarm(this))),
+      ping_alarm_(helper->CreateAlarm(new PingAlarm(this))),
       debug_visitor_(NULL),
       packet_creator_(connection_id_, &framer_, random_generator_, is_server),
       packet_generator_(this, NULL, &packet_creator_),
@@ -1058,6 +1082,7 @@ void QuicConnection::ProcessUdpPacket(const IPEndPoint& self_address,
   MaybeProcessUndecryptablePackets();
   MaybeProcessRevivedPacket();
   MaybeSendInResponseToPacket();
+  SetPingAlarm();
 }
 
 void QuicConnection::OnCanWrite() {
@@ -1407,6 +1432,7 @@ bool QuicConnection::OnPacketSent(WriteResult result) {
   if (transmission_type == NOT_RETRANSMISSION) {
     time_of_last_sent_new_packet_ = now;
   }
+  SetPingAlarm();
   DVLOG(1) << ENDPOINT << "time of last sent packet: "
            << now.ToDebuggingValue();
 
@@ -1482,6 +1508,24 @@ void QuicConnection::UpdateStopWaiting(QuicStopWaitingFrame* stop_waiting) {
       stop_waiting->least_unacked - 1);
 }
 
+void QuicConnection::SendPing() {

[jar (doing other things), 2014/04/19 16:54:15]

Per offline discussion... Please add an early return here if there is a
retransmission alarm set.  

Retransmission alarms are usually much shorter than a ping alarm duration, but I
wanted to be sure that some exponetial backoff of retransmits (perhaps even
retransmits of a ping!?!) doesn't open the door for more pings to be sent. 

[Ryan Hamilton, 2014/04/19 19:18:48]

Done.

+  if (version() <= QUIC_VERSION_17) {
+    // TODO(rch): remove this when we remove version 17.
+    // This is a horrible hideous hack which we should not support.
+    IOVector data;
+    char c_data[] = "C";
+    data.Append(c_data, 1);
+    QuicConsumedData consumed_data =
+        packet_generator_.ConsumeData(kCryptoStreamId, data, 0, false, NULL);
+    if (consumed_data.bytes_consumed == 0) {
+      DLOG(ERROR) << "Unable to send ping!?";
+    }
+  } else {
+    // TODO(rch): enable this when we merge version 18.
+    // packet_generator_.AddControlFrame(QuicFrame(new QuicPingFrame));
+  }
+}
+
 void QuicConnection::SendAck() {
   ack_alarm_->Cancel();
   stop_waiting_count_ = 0;
@@ -1802,6 +1846,20 @@ bool QuicConnection::CheckForTimeout() {
   return false;
 }
 
+void QuicConnection::SetPingAlarm() {
+  if (is_server_) {
+    // Only clients send pings.
+    return;
+  }
+  ping_alarm_->Cancel();
+  if (!visitor_->HasOpenStreams()) {
+    // Don't send a ping unless there are open streams.
+    return;
+  }
+  QuicTime::Delta ping_timeout = idle_network_timeout_.Multiply(0.5);

[jar (doing other things), 2014/04/19 16:35:43]

This trick (50% of idle timeout) is perchance OK for now... but it really should
be a separate number.

I anticipate having an idle_network_timeout_ of closer to 5 minutes.  We only
set it to 30 seconds *because* we had no ping, and the NAT will likely rebind
:-(.  Now that we have even this ghetto ping, the idle_network_timeout_ should
be independent.

I'd suggest a new value, such as ping_timeout_.  Today (in the ghetto) it should
probably default to a constant 15 seconds, but it should probably be negotiable
in the future.

If you don't want to mess with this further, could you put a TODO in at the
point you give up?  For instance, if you want to leave it as as... drop the TODO
here.

[Ryan Hamilton, 2014/04/19 19:18:48]

Done. Added a new constant for this.

+  ping_alarm_->Set(clock_->ApproximateNow().Add(ping_timeout));
+}
+
 QuicConnection::ScopedPacketBundler::ScopedPacketBundler(
     QuicConnection* connection,
     AckBundling send_ack)