Send PING to check the status of the SPDY connection.

net/spdy/spdy_session.h

Index: net/spdy/spdy_session.h
===================================================================
--- net/spdy/spdy_session.h	(revision 105385)
+++ net/spdy/spdy_session.h	(working copy)
@@ -104,7 +104,8 @@
   // NOTE:  This function can have false negatives on some platforms.
   bool VerifyDomainAuthentication(const std::string& domain);
 
-  // Send the SYN frame for |stream_id|.
+  // Send the SYN frame for |stream_id|. This also sends PING message to check
+  // the status of the connection.
   int WriteSynStream(
       spdy::SpdyStreamId stream_id,
       RequestPriority priority,
@@ -156,6 +157,14 @@
     return max_concurrent_stream_limit_;
   }
 
+  // Enable sending of PING frame with each request.
+  static void set_enable_ping_based_connection_checking(bool enable) {
+    enable_ping_based_connection_checking_ = enable;
+  }
+  static bool enable_ping_based_connection_checking() {
+    return enable_ping_based_connection_checking_;
+  }
+
   // The initial max concurrent streams per session, can be overridden by the
   // server via SETTINGS.
   static void set_init_max_concurrent_streams(size_t value) {
@@ -217,6 +226,8 @@
 
  private:
   friend class base::RefCounted<SpdySession>;
+  // Allow tests to access our innards for testing purposes.
+  FRIEND_TEST_ALL_PREFIXES(SpdySessionTest, Ping);
   FRIEND_TEST_ALL_PREFIXES(SpdySessionTest, GetActivePushStream);
 
   struct PendingCreateStream {
@@ -277,6 +288,7 @@
                  const linked_ptr<spdy::SpdyHeaderBlock>& headers);
   void OnRst(const spdy::SpdyRstStreamControlFrame& frame);
   void OnGoAway(const spdy::SpdyGoAwayControlFrame& frame);
+  void OnPing(const spdy::SpdyPingControlFrame& frame);
   void OnSettings(const spdy::SpdySettingsControlFrame& frame);
   void OnWindowUpdate(const spdy::SpdyWindowUpdateControlFrame& frame);
 
@@ -291,6 +303,31 @@
   // SETTINGS ontrol frame, update our SpdySession accordingly.
   void HandleSettings(const spdy::SpdySettings& settings);
 
+  // Send the PING (preface-PING and trailing-PING) frames.
+  void SendPrefacePingIfNoneInFlight();
+
+  // Send PING if there are no PINGs in flight and we haven't heard from server.
+  void SendPrefacePing();
+
+  // Send a PING after delay. Don't post a PING if there is already
+  // a trailing PING pending.
+  void PlanToSendTrailingPing();
+
+  // Send a PING if there is no |trailing_ping_pending_|. This PING verifies
+  // that the requests are being received by the server.
+  void SendTrailingPing();
+
+  // Send the PING frame.
+  void WritePingFrame(uint32 unique_id);
+
+  // Post a CheckPingStatus call after delay. Don't post if there is already
+  // CheckPingStatus running.
+  void PlanToCheckPingStatus();
+
+  // Check the status of the connection. It calls |CloseSessionOnError| if we
+  // haven't received any data in |kHungInterval| time period.
+  void CheckPingStatus(base::TimeTicks last_check_time);
+
   // Start reading from the socket.
   // Returns OK on success, or an error on failure.
   net::Error ReadSocket();
@@ -348,6 +385,40 @@
 
   virtual void OnDataFrameHeader(const spdy::SpdyDataFrame* frame);
 
+  // --------------------------
+  // Helper methods for testing
+  // --------------------------
+  static void set_connection_at_risk_of_loss_ms(int duration) {
+    connection_at_risk_of_loss_ms_ = duration;
+  }
+  static int connection_at_risk_of_loss_ms() {
+    return connection_at_risk_of_loss_ms_;
+  }
+
+  static void set_trailing_ping_delay_time_ms(int duration) {
+    trailing_ping_delay_time_ms_ = duration;
+  }
+  static int trailing_ping_delay_time_ms() {
+    return trailing_ping_delay_time_ms_;
+  }
+
+  static void set_hung_interval_ms(int duration) {
+    hung_interval_ms_ = duration;
+  }
+  static int hung_interval_ms() {
+    return hung_interval_ms_;
+  }
+
+  int64 pings_in_flight() const { return pings_in_flight_; }
+
+  uint32 next_ping_id() const { return next_ping_id_; }
+
+  base::TimeTicks received_data_time() const { return received_data_time_; }
+
+  bool trailing_ping_pending() const { return trailing_ping_pending_; }
+
+  bool check_ping_status_pending() const { return check_ping_status_pending_; }
+
   // Callbacks for the Spdy session.
   OldCompletionCallbackImpl<SpdySession> read_callback_;
   OldCompletionCallbackImpl<SpdySession> write_callback_;
@@ -437,6 +508,28 @@
                             // frame.
   int stalled_streams_;     // Count of streams that were ever stalled.
 
+  // Count of all pings on the wire, for which we have not gotten a response.
+  int64 pings_in_flight_;
+
+  // This is the next ping_id (unique_id) to be sent in PING frame.
+  uint32 next_ping_id_;
+
+  // This is the last time we have received data.
+  base::TimeTicks received_data_time_;
+
+  // Indicate if we have already scheduled a delayed task to send a trailing
+  // ping (and we never have more than one scheduled at a time).
+  bool trailing_ping_pending_;
+
+  // Indicate if we have already scheduled a delayed task to check the ping
+  // status.
+  bool check_ping_status_pending_;
+
+  // Indicate if the last data we sent was a ping (generally, a trailing ping).
+  // This helps us to decide if we need yet another trailing ping, or if it
+  // would be a waste of effort (and MUST not be done).
+  bool last_sent_was_ping_;
+
   // Initial send window size for the session; can be changed by an
   // arriving SETTINGS frame; newly created streams use this value for the
   // initial send window size.
@@ -457,6 +550,33 @@
   static bool use_flow_control_;
   static size_t init_max_concurrent_streams_;
   static size_t max_concurrent_stream_limit_;
+
+  // This enables or disables connection health checking system.
+  static bool enable_ping_based_connection_checking_;
+
+  // |connection_at_risk_of_loss_ms_| is an optimization to avoid sending
+  // wasteful preface pings (when we just got some data).
+  //
+  // If it is zero (the most conservative figure), then we always send the
+  // preface ping (when none are in flight).
+  //
+  // It is common for TCP/IP sessions to time out in about 3-5 minutes.
+  // Certainly if it has been more than 3 minutes, we do want to send a preface
+  // ping.
+  //
+  // We don't think any connection will time out in under about 10 seconds. So
+  // this might as well be set to something conservative like 10 seconds. Later,
+  // we could adjust it to send fewer pings perhaps.
+  static int connection_at_risk_of_loss_ms_;

[willchan no longer on Chromium, 2011/10/15 08:08:03]

Is this a new optimization? I haven't heard about this one.

Raman, in the future, please stop adding changes we haven't discussed beforehand
when we're trying to get changes in by the milestone cutoff. It would have been
much easier if we had gotten the quick and dirty change in right at the get go
and then could relax and get an updated version in later at a more leisurely
schedule.

What's the failure mode of this change? If a NAT router reboots within those 10
seconds and thus we lose the connection mapping in its table, what happens? Will
we ever catch this hang without a user-initiated refresh or something? Is that
reasonable? What about connections for non-user initiated requests? Will those
only time out after many minutes?

[jar (doing other things), 2011/10/15 09:00:52]

Recall that the back-and-forth discussion was about whether to send a preface
ping, or a follower ping. Either one was roughly sufficient, but the preface
ping was helpful in finding out sooner if the channel was long since dead, and
the follower ping was better at providing assurance that the last bytes had
reached the server (and thus justifying an arbitrary wait-for-response (with no
additional pinging).

The argument against a preface ping was that it delays (ever so slightly) the
sending of the first bits of the request.

This logic relating to line 570 simply states that IF we just very recently
received data on the wire, then the preface ping serves no valuable function,
and only delays the data.

Recall that all parties to the discussion agreed that fewer pings were better,
and this parameter allows for reducing the number of pings.  If this is set to
zero, then we always send a preface ping (when we have no current pings in
flight).  Hence it can be set to instigate maximal pinging, or to allow more
conservative (fewer) pings when starting a new session on the back (just after
finishing) receiving data from a previous session.

I think this was in the spirit of all of our discussions.



On 2011/10/15 08:08:03, willchan wrote:

[willchan no longer on Chromium, 2011/10/15 20:17:40]

Please recall http://code.google.com/p/chromium/issues/detail?id=89725#c6 which
makes no mention of this. I'm disappointed we are changing that proposal so late
in the cycle. Were we really planning to discuss this over the weekend prior to
the cutoff?

If you guys really want it, let's do it. Let's not wait another review
roundtrip, let's just get this in. That said, let me express my qualms. In
short, this is premature optimization that violates Keep It Simple Stupid and
may lead to improper handling of exceptional cases.

I feel like this is unnecessary complexity for such a little performance gain.
With the preface and trailer PING frames coming in at 12 bytes each, and the
trailer coming in one second after the preface, that means the maximum PING
overhead is 24 bytes + TLS application record overhead per second. Now, with
this change setting connection_at_risk_of_loss_ms_ to 10s, we are *at best*
saving 9 * (24 bytes + TLS application record overhead) over a 10 second period.
Over 10 seconds, do we really care about saving a few hundred bytes in the
absolute best case?

Now, if a router does reboot or whatever and we send a request out during this 9
second window and don't send anymore requests for the session, that SPDY
connection will hang and not recover until the TCP retransmit timeouts. For
pages, this isn't too big of a deal since it's likely a new request will happen
(more browsing or a refresh or something). For system requests that aren't
driven by users, this is pretty harmful. And we have plenty of those nowadays
which talk to Google over SPDY.

On 2011/10/15 09:00:52, jar wrote:

[jar (doing other things), 2011/10/15 23:25:11]

If you are concerned with it, we can set the time to zero.  It was coded with
that as a viable option.  

Raman wanted to land some histograms to see how often these things would be
useful.  

In a protocol which is deriving a lot of its benefit based on compression,
consistent care about not pushing extra messages or bytes seems to always be
prudent. ...and even if we don't choose to use the knobs, having them close at
hand may prove very helpful.

Note that the expectation is that a follower ping would be sent very soon after
the data.  I'd suggest times for follower ping transmission in the range of
500ms to 1000ms, and hence your mention of 10 seconds is perchance misplaced..

Follower-ping is just used to aggregate a few requests automatically between a
pair of pings, so omission of a preface ping is not a big deal (again, we talked
about *never* sending preface pings, and all parties were ok with it). 
Performing an optimization that sometimes (when it would IMO be embarrassing to
look at a trace and see consecutive follower-ping followed almost immediately by
a preface ping), seems like a reasonable option.

Again, if you're concerned, I'm fine with setting the
"connection_at_risk_of_loss_ms" to be 0.  That means that we're always nervous,
and always send a preface-ping even if we just got back a reply to the last
follower-ping.

Although not appropriate to this CL and goal, the whole use of pings to
stimulate (effectively) acks seems like an element of the protocol that should
have been implicitly provided at a lower level.  Specifically, we could insist
on a server sending ACKs for received data at various times, which would mean
that a ping would never be needed to stimulate such a pseudo-ACK.  Again, this
can't be in this weekend's timeline, but this should be discussed for SPDY 3.


On 2011/10/15 20:17:40, willchan wrote:

[ramant (doing other things), 2011/10/15 23:33:11]

I think it is just variable name change. In patch 14, we were using hunginterval
to send the preface ping in SendPing.

http://codereview.chromium.org/8230037/diff/25001/net/spdy/spdy_session.cc

I am not sure what should be right interval time.

[willchan no longer on Chromium, 2011/10/15 23:52:28]

Great, let's do that.
Agreed. I just don't want to miss the deadline by adding in extra complexity
that is untested (all of this timeout based code is currently untested).
You might want to re-read my comment. I said the trailer PING happens in 1
second. The 10s comes from this new knob added |connection_at_risk_of_loss_ms_|.

[willchan no longer on Chromium, 2011/10/15 23:52:28]

Oh wow, I totally missed that previously. My bad. I didn't see it as one of the
optimizations in http://code.google.com/p/chromium/issues/detail?id=89725, so I
thought it was new.

+
+  // This is the amount of time (in milliseconds) we wait before sending a
+  // trailing ping.

[willchan no longer on Chromium, 2011/10/15 08:08:03]

You should explain why we use a trailing ping.

[jar (doing other things), 2011/10/15 09:00:52]

Suggested comment:

We use a trailing ping (sent after all data) to get an effective acknowlegement
from the server that it has indeed received all (prior) data frames. With that
assurance, we are willing to enter into a wait state for responses to our last
data frame(s) without further pings.

[ramant (doing other things), 2011/10/15 23:33:11]

Done.

+  static int trailing_ping_delay_time_ms_;
+
+  // The amount of time (in milliseconds) that we are willing to tolerate with
+  // no data received (of any form), while there is a ping in flight, before we
+  // declare the connection to be hung.
+  static int hung_interval_ms_;
 };
 
 class NetLogSpdySynParameter : public NetLog::EventParameters {