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

Sent QUIC "PING" frames when a stream is open and the connection
has been idle for the half idle network timeout.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264950

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264968

[ramant (doing other things), 2014-04-18 21:20:00]

lgtm

https://codereview.chromium.org/243533003/diff/20001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/20001/net/quic/quic_connection...
net/quic/quic_connection.cc:1514:
//packet_generator_.AddControlFrame(QuicFrame(new QuicPingFrame));
nit: space between // and packet_generator_....
Should we consider adding a TODO, if we are planning on implementing it in
future?

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_session.cc...
net/quic/quic_session.cc:89: virtual bool HasOpenStreams() const override {
nit: override -> OVERRIDE

[jar (doing other things), 2014-04-18 21:57:13]

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection...
net/quic/quic_connection.cc:145: };
nit: NO_DEFAULT_COPY_AND_ASSIGN

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection...
net/quic/quic_connection.cc:1426: SetPingAlarm();
Why don't we always set this? Why do we care if it is a retransmission?

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection...
net/quic/quic_connection.cc:1516: SetPingAlarm();
Do we don't need to set it again? Shouldn't it should be set by the write, and
by the read of the responsive ack.

[Ryan Hamilton, 2014-04-19 13:55:58]

https://codereview.chromium.org/243533003/diff/20001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/20001/net/quic/quic_connection...
net/quic/quic_connection.cc:1514:
//packet_generator_.AddControlFrame(QuicFrame(new QuicPingFrame));
On 2014/04/18 21:20:01, ramant wrote:
> nit: space between // and packet_generator_....

Done.

> Should we consider adding a TODO, if we are planning on implementing it in
> future?

Sure.  There's an internal CL to merge which should enable this.

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection...
net/quic/quic_connection.cc:145: };
On 2014/04/18 21:57:13, jar wrote:
> nit: NO_DEFAULT_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection...
net/quic/quic_connection.cc:1426: SetPingAlarm();
On 2014/04/18 21:57:13, jar wrote:
> Why don't we always set this? Why do we care if it is a retransmission?

Done. I was keeping it in sync with the timeout alarm, which is based on the
time_of_last_sent_new_packet_ member. But as you point out, we don't care about
that for the purposes of the ping alarm. As a result, I've also change the
SetPingAlarm method to set based on Now().

https://codereview.chromium.org/243533003/diff/60001/net/quic/quic_connection...
net/quic/quic_connection.cc:1516: SetPingAlarm();
On 2014/04/18 21:57:13, jar wrote:
> Do we don't need to set it again? Shouldn't it should be set by the write, and
> by the read of the responsive ack.

Interesting! Yes, good point, the send of the PING should reset the alarm.

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

I was wondering about retransmission... as I think you were.  My fear was that a
retry interval, after exponential backoff, could get so long, that we might
<sigh> decide to send a ping (maybe even "another ping" if the retransmits were
for a ping to start with!?!).  

I liked what came up in off-line discussion, about throttling pings when there
is a plan to retransmit something (anyway).

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection.cc:1859: QuicTime::Delta ping_timeout =
idle_network_timeout_.Multiply(0.5);
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.

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_session.c...
net/quic/quic_session.cc:309: return GetNumOpenStreams() > 0;
How does the crypto stream fit into the mix?  It is probably open... but we
shouldn't consider it for this purpose.  Is it a "special" stream and excluded
from that count?  Similarly for the header stream.

Maybe the name should be different.  Maybe:
HasClosableStreams()

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

With the early return (discussed offline, and below) added, and the other small
comments I sent (rename;  todo and/or distinct ping timeout constant)
addressed...

LGTM

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection.cc:1511: void QuicConnection::SendPing() {
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]

Thanks for the weekend review.

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection.cc:1511: void QuicConnection::SendPing() {
On 2014/04/19 16:54:15, jar wrote:
> 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. 

Done.

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection.cc:1859: QuicTime::Delta ping_timeout =
idle_network_timeout_.Multiply(0.5);
On 2014/04/19 16:35:43, jar wrote:
> 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.

Done. Added a new constant for this.

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/243533003/diff/160001/net/quic/quic_session.c...
net/quic/quic_session.cc:309: return GetNumOpenStreams() > 0;
On 2014/04/19 16:35:43, jar wrote:
> How does the crypto stream fit into the mix?  It is probably open... but we
> shouldn't consider it for this purpose.  Is it a "special" stream and excluded
> from that count?  Similarly for the header stream.
> 
> Maybe the name should be different.  Maybe:
> HasClosableStreams() 

Both the crypto and headers streams are not considered "open" in this context.
That being said, I've renamed the method to HasOpenDataStreams to be consistent
with the QuicDataStream class (which exclude crypto and headers streams.

[Ryan Hamilton, 2014-04-19 19:18:53]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-04-19 19:19:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/243533003/180001

[commit-bot: I haz the power, 2014-04-19 19:44:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-19 19:44:33]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_gn_rel

[Ryan Hamilton, 2014-04-20 02:36:04]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-04-20 02:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/243533003/200001

[commit-bot: I haz the power, 2014-04-20 22:13:25]

Change committed as 264950

[Ryan Hamilton, 2014-04-21 01:50:29]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-04-21 01:50:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/243533003/210001

[commit-bot: I haz the power, 2014-04-21 06:26:59]

Change committed as 264968