Quic use reduced PING timeout when previous connection times out with open streams

Quic use reduced PING timeout when previous connection times out with open streams
the reduced ping timeout is controlled by a field trial param.

BUG=

Committed: https://crrev.com/dd1439f61807327301c7a0aacc3cb30802529280
Cr-Commit-Position: refs/heads/master@{#416162}

[Zhongyi Shi, 2016-08-31 03:27:47]

The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 03:28:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2016-08-31 03:28:49]

zhongyi@chromium.org changed reviewers:
+ ianswett@chromium.org, rch@chromium.org

[commit-bot: I haz the power, 2016-08-31 03:47:10]

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

[commit-bot: I haz the power, 2016-08-31 03:47:12]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Ryan Hamilton, 2016-09-01 16:36:14]

looking good. A couple of nits.

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_chro...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_chro...
net/quic/chromium/quic_chromium_client_session.cc:882:
stream_factory_->set_reduced_ping_timeout();
nit: I'd probably be inclined to call stream_factory_->OnTimeoutWithOpenStream()
and then the factory can decide what to do with this information.

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.h:606: int reduced_ping_timeout_seconds_;
nit: Let's make these QuicTime::Deltas

[Zhongyi Shi, 2016-09-01 18:58:30]

Ryan: ptal, thanks!

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_chro...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_chro...
net/quic/chromium/quic_chromium_client_session.cc:882:
stream_factory_->set_reduced_ping_timeout();
On 2016/09/01 16:36:14, Ryan Hamilton wrote:
> nit: I'd probably be inclined to call
stream_factory_->OnTimeoutWithOpenStream()
> and then the factory can decide what to do with this information.

Done.

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_stre...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2301453002/diff/1/net/quic/chromium/quic_stre...
net/quic/chromium/quic_stream_factory.h:606: int reduced_ping_timeout_seconds_;
On 2016/09/01 16:36:14, Ryan Hamilton wrote:
> nit: Let's make these QuicTime::Deltas

Done.

[Ryan Hamilton, 2016-09-01 19:52:15]

lgtm

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:879: // Notify the factory the
connection times out with open streams.
nit: times->timed.

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.h:258: // Called by a session when
connection times out with open streams.
nit: s/connection/it/

[ianswett, 2016-09-01 20:04:15]

One high level question.  It appears this occurs on timeouts from open streams,
which seems good.

But we've seen cases when streams didn't timeout, but we saw ridiculous packet
loss after some point, then sent a packet, and then saw no packet loss for a
period of time.  These are cases when this would be most valuable, but the
existing condition doesn't detect these.

https://codereview.chromium.org/2301453002/diff/20001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2301453002/diff/20001/net/http/http_network_s...
net/http/http_network_session.cc:125:
quic_reduced_ping_timeout_seconds(kPingTimeoutSecs),
nit: kDefaultPingTimeoutSecs?

[Zhongyi Shi, 2016-09-02 00:48:05]

On 2016/09/01 20:04:15, ianswett wrote:
> One high level question.  It appears this occurs on timeouts from open
streams,
> which seems good.
> 
> But we've seen cases when streams didn't timeout, but we saw ridiculous packet
> loss after some point, then sent a packet, and then saw no packet loss for a
> period of time.  These are cases when this would be most valuable, but the
> existing condition doesn't detect these.

We could definitely take a look in that case. How often do we see that happen
and could you help upload the net-internals to the bug tracker? Thanks. I am
going to submit this now and will upload a new CL if future work needed for that
specific case.

[Zhongyi Shi, 2016-09-02 00:48:21]

https://codereview.chromium.org/2301453002/diff/20001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2301453002/diff/20001/net/http/http_network_s...
net/http/http_network_session.cc:125:
quic_reduced_ping_timeout_seconds(kPingTimeoutSecs),
On 2016/09/01 20:04:15, ianswett wrote:
> nit: kDefaultPingTimeoutSecs?

Shall we change it when we decide to default this on?

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.cc:879: // Notify the factory the
connection times out with open streams.
On 2016/09/01 19:52:15, Ryan Hamilton wrote:
> nit: times->timed.

Done.

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2301453002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.h:258: // Called by a session when
connection times out with open streams.
On 2016/09/01 19:52:15, Ryan Hamilton wrote:
> nit: s/connection/it/

Done.

[Zhongyi Shi, 2016-09-02 00:48:32]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-09-02 00:48:32]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2301453002/#ps40001
(title: "address nits")

[commit-bot: I haz the power, 2016-09-02 00:48:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 02:02:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 02:04:23]

Description was changed from

==========
Quic use reduced PING timeout when previous connection times out with open
streams
the reduced ping timeout is controlled by a field trial param.

BUG=
==========

to

==========
Quic use reduced PING timeout when previous connection times out with open
streams
the reduced ping timeout is controlled by a field trial param.

BUG=

Committed: https://crrev.com/dd1439f61807327301c7a0aacc3cb30802529280
Cr-Commit-Position: refs/heads/master@{#416162}
==========

[commit-bot: I haz the power, 2016-09-02 02:04:24]

Patchset 3 (id:??) landed as
https://crrev.com/dd1439f61807327301c7a0aacc3cb30802529280
Cr-Commit-Position: refs/heads/master@{#416162}