When an idle socket is added back to a socket pool, check for stalled jobs in lower pools

When an idle socket is added back to a socket pool,
check if lower layer socket pools are stalled.  If so,
close the idle socket.

Also, when a SPDY stream is destroyed, check if
the session is idle and a lower layer pool is stalled,
and close the session if needed.

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

[mmenke, 2013-07-16 16:15:12]

rch:  PTAL

willchan:  FYI

This is the final part of closing idle sockets when lower layer pools need them.
 This needs higher level pools to be able to query lower level pools to check if
they're stalled.  Most of the CL is plumbing that through.

One slight oddity is that if you have a stalled higher level pool on a stalled
lower level pool, if the higher level pool is stalled with a higher priority
group than the lower level pool, the lower level pool will still get the socket,
but that's the same behavior as before, and uses the same codepath as before. 
Doesn't seem worth the effort of fixing.

[Ryan Hamilton, 2013-07-21 14:53:54]

This looks like a great change.  I like the explicit Higher/Lower naming choice.
 One comment, lgtm otherwise.  (Sorry for the delay)

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:247: // If a lower layer pool is stalled,
consider |this| stalled as well.
This seems like mostly a good idea, but how does it work with SPDY sessions in
which case, the transport socket pool can be stalled but the SpdySession can
still vend new streams?

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

https://codereview.chromium.org/18796003/diff/44022/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:1005: RemoveFromPool();
akalin has been refactoring SpdySession quite a bit recently.  You might need to
rebase, I'm not sure.

[mmenke, 2013-07-22 20:13:33]

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:247: // If a lower layer pool is stalled,
consider |this| stalled as well.
On 2013/07/21 14:53:54, Ryan Hamilton wrote:
> This seems like mostly a good idea, but how does it work with SPDY sessions in
> which case, the transport socket pool can be stalled but the SpdySession can
> still vend new streams?

SpdySessions can never be stalled.  They aren't considered LowerLayeredPools, so
they don't need to implement this function, conveniently enough.

Anything layered on top of a SpdySession works completely differently from a
socket pool - since it can always get streams from the session, it can't be
stalled (Unless there's a stream limit on the session, but that's beyond the
scope of this CL).

That having been said, SpdySessions can result in other requests (Possibly
requests needed to create other SpdySessions) can be stalled, of course, and so
if a session becomes idle, it will be closed if there are stalled lower level
pools.

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

https://codereview.chromium.org/18796003/diff/44022/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:1005: RemoveFromPool();
On 2013/07/21 14:53:54, Ryan Hamilton wrote:
> akalin has been refactoring SpdySession quite a bit recently.  You might need
to
> rebase, I'm not sure.

Yea, this completely doesn't work now.  Removing from the pool now deletes the
session, but a lot of methods run code after CloseActiveStreamIterator.

I'll have to get a better understanding of SpdySessions to figure out how to
update the CL.

[Ryan Hamilton, 2013-07-22 20:22:46]

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:247: // If a lower layer pool is stalled,
consider |this| stalled as well.
On 2013/07/22 20:13:33, mmenke wrote:
> On 2013/07/21 14:53:54, Ryan Hamilton wrote:
> > This seems like mostly a good idea, but how does it work with SPDY sessions
in
> > which case, the transport socket pool can be stalled but the SpdySession can
> > still vend new streams?
> 
> SpdySessions can never be stalled.  They aren't considered LowerLayeredPools,
so
> they don't need to implement this function, conveniently enough.
> 
> Anything layered on top of a SpdySession works completely differently from a
> socket pool - since it can always get streams from the session, it can't be
> stalled (Unless there's a stream limit on the session, but that's beyond the
> scope of this CL).

Works for me.  FWIW, there is a streams/session limit which I think is typically
100.  Normally this isn't much of a problem.  Though in the case of a SPDY Proxy
it might potentially be one.  But no reason to worry about that in this CL, I
think.
 
> That having been said, SpdySessions can result in other requests (Possibly
> requests needed to create other SpdySessions) can be stalled, of course, and
so
> if a session becomes idle, it will be closed if there are stalled lower level
> pools.

[mmenke, 2013-07-24 19:05:23]

Here's a very simple workaround for safe when it becomes idle.  I don't think
destroying the session synchronously is worth the extra complexity involved.

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/44022/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:247: // If a lower layer pool is stalled,
consider |this| stalled as well.
On 2013/07/22 20:22:46, Ryan Hamilton wrote:
> On 2013/07/22 20:13:33, mmenke wrote:
> > On 2013/07/21 14:53:54, Ryan Hamilton wrote:
> > > This seems like mostly a good idea, but how does it work with SPDY
sessions
> in
> > > which case, the transport socket pool can be stalled but the SpdySession
can
> > > still vend new streams?
> > 
> > SpdySessions can never be stalled.  They aren't considered
LowerLayeredPools,
> so
> > they don't need to implement this function, conveniently enough.
> > 
> > Anything layered on top of a SpdySession works completely differently from a
> > socket pool - since it can always get streams from the session, it can't be
> > stalled (Unless there's a stream limit on the session, but that's beyond the
> > scope of this CL).
> 
> Works for me.  FWIW, there is a streams/session limit which I think is
typically
> 100.  Normally this isn't much of a problem.  Though in the case of a SPDY
Proxy
> it might potentially be one.  But no reason to worry about that in this CL, I
> think.

Yea, I don't think so - that would need to be dealt with just between the
SpdySession and and the things on top of it.  No need for it to dig down into
the socket pool, unless we wanted to use multiple SpdySessions at that point
(And I assume we wouldn't want to do that, since that defeats the point of the
stream limit in the first place).

[Ryan Hamilton, 2013-07-24 19:59:00]

+akalin

[mmenke, 2013-08-19 13:19:42]

On 2013/07/24 19:59:00, Ryan Hamilton wrote:
> +akalin

akalin:  Ping!

[akalin, 2013-08-21 17:02:14]

On 2013/08/19 13:19:42, mmenke wrote:
> On 2013/07/24 19:59:00, Ryan Hamilton wrote:
> > +akalin
> 
> akalin:  Ping!

sorry, didn't see this! looking now

[mmenke, 2013-08-21 17:04:00]

On 2013/08/21 17:02:14, akalin wrote:
> On 2013/08/19 13:19:42, mmenke wrote:
> > On 2013/07/24 19:59:00, Ryan Hamilton wrote:
> > > +akalin
> > 
> > akalin:  Ping!
> 
> sorry, didn't see this! looking now

No problem.  I haven't been in a hurry, or would have been more aggressive about
it.  The only part that you really need to look at is the SPDY stuff (Though
happy to head feedback from the rest as well).

[akalin, 2013-08-21 17:29:03]

initial comments

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:259: // If we are not using
|max_sockets_|, then clearly we are not stalled
period at end

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
net/socket/client_socket_pool_base.h:605: // Pools that create connections
through |this|.  |this| will Try to close
Try -> try

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

https://codereview.chromium.org/18796003/diff/167001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1051:
unclaimed_pushed_streams_.erase(owned_stream->url());
can you fix this indent while you're here?

https://codereview.chromium.org/18796003/diff/167001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1056: if (active_streams_.empty() &&
availability_state_ == STATE_AVAILABLE &&
i think this should go after DeleteStream, as otherwise the stream's delegate
won't get called in some cases (if we call CloseIfIdle synchronously -- see
below).

You can do the same trick that DeleteStream() does, and also check if it's
already closed:

WeakPtr<SpdySession> weak_this = GetWeakPtr();

DeleteStream(...)

if (!weak_this)
  return;

if (availability_state_ == STATE_CLOSED)
  return;

if (active_streams_.empty() && connection_->IsPoolStalled())
  ...

https://codereview.chromium.org/18796003/diff/167001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1059: base::MessageLoop::current()->PostTask(
you should be able to just call CloseIfIdle here directly -- callers should
definitely expect that closing an active stream can close the session, too.

[mmenke, 2013-08-21 18:17:16]

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:259: // If we are not using
|max_sockets_|, then clearly we are not stalled
On 2013/08/21 17:29:03, akalin wrote:
> period at end

Done.

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/18796003/diff/167001/net/socket/client_socket...
net/socket/client_socket_pool_base.h:605: // Pools that create connections
through |this|.  |this| will Try to close
On 2013/08/21 17:29:03, akalin wrote:
> Try -> try

Done.

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

https://codereview.chromium.org/18796003/diff/167001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1051:
unclaimed_pushed_streams_.erase(owned_stream->url());
On 2013/08/21 17:29:03, akalin wrote:
> can you fix this indent while you're here?

Done.

https://codereview.chromium.org/18796003/diff/167001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1056: if (active_streams_.empty() &&
availability_state_ == STATE_AVAILABLE &&
On 2013/08/21 17:29:03, akalin wrote:
> i think this should go after DeleteStream, as otherwise the stream's delegate
> won't get called in some cases (if we call CloseIfIdle synchronously -- see
> below).
> 
> You can do the same trick that DeleteStream() does, and also check if it's
> already closed:
> 
> WeakPtr<SpdySession> weak_this = GetWeakPtr();
> 
> DeleteStream(...)
> 
> if (!weak_this)
>   return;
> 
> if (availability_state_ == STATE_CLOSED)
>   return;
> 
> if (active_streams_.empty() && connection_->IsPoolStalled())
>   ...

Done.

https://codereview.chromium.org/18796003/diff/167001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1059: base::MessageLoop::current()->PostTask(
On 2013/08/21 17:29:03, akalin wrote:
> you should be able to just call CloseIfIdle here directly -- callers should
> definitely expect that closing an active stream can close the session, too.

Done.

When I wrote this CL a month ago, that resulted in crashing.  When a stream was
shut down (cancelled?), it would delete the stream, and then write the stream
closed message to the socket.  Deleting the session here in that case would
cause a crash.  This no longer seems to be the case.

[akalin, 2013-08-21 21:01:17]

couple more SPDY comments

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

https://codereview.chromium.org/18796003/diff/198001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1063: // If there are no active streams, and the socket
pool is stalled, try to free
I think the comment could be a bit clearer:

// If there are no active streams and the socket pool is stalled,
// close the session to free up a socket slot.

https://codereview.chromium.org/18796003/diff/198001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1066: CloseIfIdle();
can you remove CloseIfIdle and just inline the code here? I imagine something
like this would work:

if (active_streams...) {
  CloseSessionResult result = DoCloseSession(...);
  DCHECK_NE(result, SESSION_ALREADY_CLOSED);
}

[mmenke, 2013-08-21 21:10:08]

Thanks for the feedback.

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

https://codereview.chromium.org/18796003/diff/198001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1063: // If there are no active streams, and the socket
pool is stalled, try to free
On 2013/08/21 21:01:18, akalin wrote:
> I think the comment could be a bit clearer:
> 
> // If there are no active streams and the socket pool is stalled,
> // close the session to free up a socket slot.

Done.

https://codereview.chromium.org/18796003/diff/198001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:1066: CloseIfIdle();
On 2013/08/21 21:01:18, akalin wrote:
> can you remove CloseIfIdle and just inline the code here? I imagine something
> like this would work:
> 
> if (active_streams...) {
>   CloseSessionResult result = DoCloseSession(...);
>   DCHECK_NE(result, SESSION_ALREADY_CLOSED);
> }

Done.  I also got rid of the check that pool_ is non-NULL, as CloseIfIdle
already seems to handle that case.

[akalin, 2013-08-21 22:36:31]

SPDY stuff looks good, but a few more comments

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
File net/socket/client_socket_handle.cc (right):

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:89: CHECK(higher_pool);
can you add comments in the .h file saying that at most one HLP can be added to
the handle at a time?

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:91: if (pool_) {
calling this function when pool_ is NULL seems like an error to me. Is it? If
so, change this into a check maybe?

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:97: void
ClientSocketHandle::RemoveHigherLayeredPool(
may as well add a comment in the .h file saying that the passed-in pool must
match the one passed in to AddHigherLayeredPool().

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:101: if (pool_) {
same comment re. pool_ being NULL

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_pool_base.h:216: // Adds a lower layered pool to
|this|, and adds |pool_| as a higher layered
pool_ -> this

[mmenke, 2013-08-22 14:35:31]

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
File net/socket/client_socket_handle.cc (right):

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:89: CHECK(higher_pool);
On 2013/08/21 22:36:31, akalin wrote:
> can you add comments in the .h file saying that at most one HLP can be added
to
> the handle at a time?

Done.

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:91: if (pool_) {
On 2013/08/21 22:36:31, akalin wrote:
> calling this function when pool_ is NULL seems like an error to me. Is it? If
> so, change this into a check maybe?

CreateFakeSpdySessionHelper in spdy_test_util_common.cc creates
ClientSocketHandles used for SPDY tests without SocketPools, and also results in
calling this function.  Think changing that is beyond the scope of this CL.

I've added a TODO about changing that.

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:97: void
ClientSocketHandle::RemoveHigherLayeredPool(
On 2013/08/21 22:36:31, akalin wrote:
> may as well add a comment in the .h file saying that the passed-in pool must
> match the one passed in to AddHigherLayeredPool().

Done.

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_handle.cc:101: if (pool_) {
On 2013/08/21 22:36:31, akalin wrote:
> same comment re. pool_ being NULL

While I believe this code will be skipped in the case mentioned above, I think
the two should match, to just in case when/where RemoveHigherLayeredPool is
called changes.

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/18796003/diff/217001/net/socket/client_socket...
net/socket/client_socket_pool_base.h:216: // Adds a lower layered pool to
|this|, and adds |pool_| as a higher layered
On 2013/08/21 22:36:31, akalin wrote:
> pool_ -> this

Done.

[akalin, 2013-08-22 19:23:25]

lgtm!

https://codereview.chromium.org/18796003/diff/244001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/244001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:259: // If we are not using
|max_sockets_|, then clearly we are not stalled.
"not using |max_sockets_|" can be interpreted as "not using the variable
max_sockets_".  Perhaps "If we are using fewer than |max_sockets_| sockets, "

https://codereview.chromium.org/18796003/diff/244001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:262: // So in order to be stalled we need
to be using |max_sockets_| AND
same thing here? (although I realize it's kinda awkward language)

[mmenke, 2013-08-22 19:53:05]

Thanks so much for the comments!

https://codereview.chromium.org/18796003/diff/244001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/18796003/diff/244001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:259: // If we are not using
|max_sockets_|, then clearly we are not stalled.
On 2013/08/22 19:23:25, akalin wrote:
> "not using |max_sockets_|" can be interpreted as "not using the variable
> max_sockets_".  Perhaps "If we are using fewer than |max_sockets_| sockets, "

Done.  I also removed use of "we" (This was actually an old comment, but as long
as I'm updating it, using "we" bothers some people).

https://codereview.chromium.org/18796003/diff/244001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:262: // So in order to be stalled we need
to be using |max_sockets_| AND
On 2013/08/22 19:23:25, akalin wrote:
> same thing here? (although I realize it's kinda awkward language)

Done (-> "using at least |max_sockets_|").  Also got rid of "we".

[commit-bot: I haz the power, 2013-08-22 19:56:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/18796003/192001

[commit-bot: I haz the power, 2013-08-22 23:41:55]

Change committed as 219147