Close idle H2 sockets when SpdySession is initialized.

Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind feature flag: 
CloseIdleH2SocketsEarly.

BUG=690918
Review-Url: https://codereview.chromium.org/2678353003
Cr-Commit-Position: refs/heads/master@{#454607}
Committed: https://chromium.googlesource.com/chromium/src/+/92feb33d4dfa486c134e10cec36ff8b85e31d3aa

[xunjieli, 2017-02-07 19:01:36]

Description was changed from

==========
Close idle H2 sockets when SpdySession is initialized.

BUG=621597
==========

to

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while in socket
pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use those idle
sockets again.

This new behavior is hidden behind a feature flag.

BUG=621597
==========

[xunjieli, 2017-02-07 19:02:00]

Description was changed from

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while in socket
pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use those idle
sockets again.

This new behavior is hidden behind a feature flag.

BUG=621597
==========

to

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind a feature flag.

BUG=621597
==========

[xunjieli, 2017-02-07 20:57:35]

Patchset #1 (id:1) has been deleted

[xunjieli, 2017-02-07 20:57:42]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2017-02-07 20:57:48]

Patchset #1 (id:40001) has been deleted

[xunjieli, 2017-02-07 20:57:55]

Patchset #1 (id:60001) has been deleted

[xunjieli, 2017-02-07 20:58:37]

xunjieli@chromium.org changed reviewers:
+ bnc@chromium.org, davidben@chromium.org

[xunjieli, 2017-02-07 20:58:37]

Bence and David: PTAL. Thanks.

[xunjieli, 2017-02-07 20:58:54]

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

[commit-bot: I haz the power, 2017-02-07 20:59:31]

Dry run: 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, 2017-02-07 22:19:14]

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

[commit-bot: I haz the power, 2017-02-07 22:19:15]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-02-08 00:18:51]

bnc@chromium.org changed required reviewers:
+ davidben@chromium.org

[Bence, 2017-02-08 00:18:52]

LGTM with nits, but I am not very familiar with socket pools, so please wait for
davidben@'s review.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:838: // this Http2 connection.
s/'Http2'/'HTTP/2'/

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:291: // |spdy_session_key| is the host/port that this
session connects to, privacy
Optional: remove lines 291--294, because they really do not add any value.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6023: std::unique_ptr<base::FeatureList>
feature_list(new base::FeatureList);
Use auto and base::MakeUnique as suggested by
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?l=48.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6026: GetParam() ?
base::FeatureList::OVERRIDE_ENABLE_FEATURE
Add a protected const bool member to save GetParam() value.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6058: // Create some Http/2 sockets.
Nit: HTTP/2 is usually written in all uppercase.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6066: std::unique_ptr<ClientSocketHandle>
connection(new ClientSocketHandle);
base::MakeUnique<>

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6071: new SSLSocketParams(transport_params,
NULL, NULL, key_.host_port_pair(),
s/NULL/nullptr/g

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6093: GetParam() ? 0 : (int)kNumIdleSockets -
1,
Use protected const bool member instead of GetParam().

[xunjieli, 2017-02-08 14:15:40]

Thanks! David, PTAL.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:838: // this Http2 connection.
On 2017/02/08 00:18:52, Bence wrote:
> s/'Http2'/'HTTP/2'/

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:291: // |spdy_session_key| is the host/port that this
session connects to, privacy
On 2017/02/08 00:18:52, Bence wrote:
> Optional: remove lines 291--294, because they really do not add any value.

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6023: std::unique_ptr<base::FeatureList>
feature_list(new base::FeatureList);
On 2017/02/08 00:18:52, Bence wrote:
> Use auto and base::MakeUnique as suggested by
> https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?l=48.

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6026: GetParam() ?
base::FeatureList::OVERRIDE_ENABLE_FEATURE
On 2017/02/08 00:18:52, Bence wrote:
> Add a protected const bool member to save GetParam() value.

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6058: // Create some Http/2 sockets.
On 2017/02/08 00:18:52, Bence wrote:
> Nit: HTTP/2 is usually written in all uppercase.

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6066: std::unique_ptr<ClientSocketHandle>
connection(new ClientSocketHandle);
On 2017/02/08 00:18:52, Bence wrote:
> base::MakeUnique<>

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6071: new SSLSocketParams(transport_params,
NULL, NULL, key_.host_port_pair(),
On 2017/02/08 00:18:52, Bence wrote:
> s/NULL/nullptr/g

Done.

https://codereview.chromium.org/2678353003/diff/80001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:6093: GetParam() ? 0 : (int)kNumIdleSockets -
1,
On 2017/02/08 00:18:52, Bence wrote:
> Use protected const bool member instead of GetParam().

Done.

[xunjieli, 2017-02-08 14:16:07]

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

[commit-bot: I haz the power, 2017-02-08 14:16:34]

Dry run: 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, 2017-02-08 15:18:10]

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

[commit-bot: I haz the power, 2017-02-08 15:18:11]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-02-08 16:47:15]

Description was changed from

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind a feature flag.

BUG=621597
==========

to

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind a feature flag.

BUG=524258,621597
==========

[xunjieli, 2017-02-10 13:50:56]

Description was changed from

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind a feature flag.

BUG=524258,621597
==========

to

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind a feature flag.

BUG=690918
==========

[xunjieli, 2017-02-10 13:51:55]

Description was changed from

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind a feature flag.

BUG=690918
==========

to

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind feature flag: 
CloseIdleH2SocketsEarly.

BUG=690918
==========

[davidben, 2017-03-02 22:00:52]

Just a few nits and one comment below which would hopefully make a few things
tidier.

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:617: base::TimeTicks now =
base::TimeTicks::Now();
Super-nitpicky nit: You can probably just pass that into
CleanupIdleSocketsInGroup directly without the variable.

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:156: // sockets may release stuff
to the lower level pools.
Nit: it => pool or pair or something? (With a loop like this, these aren't
iterators anymore.)

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:183: const std::string&
group_name) {
This will close idle sockets with the same group name in all pools, rather than
just the one that the socket was made in. Is that quite right? I think group
names might only be unique within a single pool?

I see the SpdySession code is using ClientSocketHandle::group_name to get the
group name out. The ClientSocketHandle also has a pool_ pointer back to the
pool. Seems you could just do:

void ClientSocketHandle::CloseIdleSocketsInGroup() {
  if (pool_)
    pool_->CloseIdleSocketsInGroup(group_name_);
}

connection_->CloseIdleSocketsInGroup();

This would also save you from routing HttpNetworkSession into everything. :-)

[davidben, 2017-03-02 22:05:52]

(Oh, sorry, missed a file. Just tiny nitpicks.)

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:6037: size_t kNumIdleSockets = 4;
Nit: const

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:6077: (int)kNumIdleSockets,
Nit: If you make kNumIdleSockets an int, you can probably save all these casts.

(Otherwise (int) => static_cast<int>.)

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:6083: 
Nit: maybe add

// If the experiment is enabled, the SpdySession will close the idle sockets.

[xunjieli, 2017-03-02 23:01:20]

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

[commit-bot: I haz the power, 2017-03-02 23:02:04]

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

[xunjieli, 2017-03-02 23:04:01]

Thanks!

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:617: base::TimeTicks now =
base::TimeTicks::Now();
On 2017/03/02 22:00:51, davidben wrote:
> Super-nitpicky nit: You can probably just pass that into
> CleanupIdleSocketsInGroup directly without the variable.

Done.

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:156: // sockets may release stuff
to the lower level pools.
On 2017/03/02 22:00:51, davidben wrote:
> Nit: it => pool or pair or something? (With a loop like this, these aren't
> iterators anymore.)

Acknowledged. No longer apply :)

https://codereview.chromium.org/2678353003/diff/120001/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.cc:183: const std::string&
group_name) {
On 2017/03/02 22:00:51, davidben wrote:
> This will close idle sockets with the same group name in all pools, rather
than
> just the one that the socket was made in. Is that quite right? I think group
> names might only be unique within a single pool?
> 
> I see the SpdySession code is using ClientSocketHandle::group_name to get the
> group name out. The ClientSocketHandle also has a pool_ pointer back to the
> pool. Seems you could just do:
> 
> void ClientSocketHandle::CloseIdleSocketsInGroup() {
>   if (pool_)
>     pool_->CloseIdleSocketsInGroup(group_name_);
> }
> 
> connection_->CloseIdleSocketsInGroup();
> 
> This would also save you from routing HttpNetworkSession into everything. :-)

That's very neat! Done. Thanks for the suggestion.

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:6037: size_t kNumIdleSockets = 4;
On 2017/03/02 22:05:52, davidben wrote:
> Nit: const

Done.

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:6077: (int)kNumIdleSockets,
On 2017/03/02 22:05:52, davidben wrote:
> Nit: If you make kNumIdleSockets an int, you can probably save all these
casts.
> 
> (Otherwise (int) => static_cast<int>.)

Done.

https://codereview.chromium.org/2678353003/diff/120001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:6083: 
On 2017/03/02 22:05:52, davidben wrote:
> Nit: maybe add
> 
> // If the experiment is enabled, the SpdySession will close the idle sockets.

Done.

[commit-bot: I haz the power, 2017-03-03 00:04:16]

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

[commit-bot: I haz the power, 2017-03-03 00:04:17]

Dry run: This issue passed the CQ dry run.

[davidben, 2017-03-03 03:16:36]

lgtm

[xunjieli, 2017-03-03 17:14:22]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-03-03 17:14:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/2678353003/#ps160001
(title: "Address comments to hook directly to ClientSocketHandle")

[commit-bot: I haz the power, 2017-03-03 17:14: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, 2017-03-03 17:19:31]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488561262619500,
"parent_rev": "d75f748f806d61b22012c723d63322afb2ff7208", "commit_rev":
"92feb33d4dfa486c134e10cec36ff8b85e31d3aa"}

[commit-bot: I haz the power, 2017-03-03 17:20:13]

Description was changed from

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind feature flag: 
CloseIdleH2SocketsEarly.

BUG=690918
==========

to

==========
Close idle H2 sockets when SpdySession is initialized.

Once a SpdySession is initialized, subsequent H2 requests to the same 
host will reuse the connection, bypassing socket pool logic. Therefore, 
if we have multiple H2 idle sockets, they can hang around for a while 
in socket pools. This CL tries to clean up these idle H2
sockets when a SpdySession is initialized, since we will never use 
those idle sockets again.

This new behavior is hidden behind feature flag: 
CloseIdleH2SocketsEarly.

BUG=690918

Review-Url: https://codereview.chromium.org/2678353003
Cr-Commit-Position: refs/heads/master@{#454607}
Committed:
https://chromium.googlesource.com/chromium/src/+/92feb33d4dfa486c134e10cec36f...
==========

[commit-bot: I haz the power, 2017-03-03 17:20:14]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/92feb33d4dfa486c134e10cec36f...