Revert 113300 - Revert of 112134 of Revert 112130 - Close idle connections / SPDY sessions when n...

Revert 113305 - Revert 113300 - Revert of 112134 of Revert 112130 - Close idle connections / SPDY sessions when needed.

Due to the idle connection state being held by different socket pools, it's possible for one socket pool to hold an idle socket in a lower layer socket pool. From the lower level socket pool's perspective, the socket is being "actively" used. From the higher socket pool's (including SpdySession, which is more of a connection manager) perspective, the connection is idle and can be closed if we have hit a limit.

Normally this isn't a big deal, except when we have a lot of idle SPDY connections and are connecting via a proxy, so we have low connection limits through the proxy server. We address this problem by allowing lower-level socket pools to tell higher level socket pools to close a socket.

Fixed ASAN test failures by removing .Times(1) and .Times(2) from CloseMultipleIdleSocketsHeldByLayeredPoolWhenNeeded unittest (this removes the tests relying on the order of std::set in CloseOneIdleConnectionInLayeredPool). ASAN is prob
ably causing the memory allocator to allocate the pools differently. The std::set is ordered by LayeredPool* which is the address of the LayeredPool (willchan).

Added NET_EXPORT for layered_pool class defintion to fix windows shared compile.

BUG=62364, 92244, 105839
TEST=none


Review URL: http://codereview.chromium.org/8340012

TBR=willchan@chromium.org
Review URL: http://codereview.chromium.org/8745007

TBR=willchan@chromium.org

Review URL: http://codereview.chromium.org/8803019

TBR=rtenneti@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113305

TBR=willchan@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113405

[ramant (doing other things), 2011-12-07 03:36:38]

Added NET_EXPORT to LayeredPool class to fix win shared build.   I am at fault
for not compiling windows and linux shared builds. Compiling on all try bots
now.

thanks,
raman

[willchan no longer on Chromium, 2011-12-07 03:39:15]

LGTM

On Tue, Dec 6, 2011 at 7:36 PM, <rtenneti@chromium.org> wrote:

> Reviewers: willchan,
>
> Message:
> Added NET_EXPORT to LayeredPool class to fix win shared build.   I am at
> fault
> for not compiling windows and linux shared builds. Compiling on all try
> bots
> now.
>
> thanks,
> raman
>
> Description:
> Revert 113305 - Revert 113300 - Revert of 112134 of Revert 112130 - Close
> idle
> connections / SPDY sessions when needed.
>
> Due to the idle connection state being held by different socket pools, it's
> possible for one socket pool to hold an idle socket in a lower layer socket
> pool. From the lower level socket pool's perspective, the socket is being
> "actively" used. From the higher socket pool's (including SpdySession,
> which is
> more of a connection manager) perspective, the connection is idle and can
> be
> closed if we have hit a limit.
>
> Normally this isn't a big deal, except when we have a lot of idle SPDY
> connections and are connecting via a proxy, so we have low connection
> limits
> through the proxy server. We address this problem by allowing lower-level
> socket
> pools to tell higher level socket pools to close a socket.
>
> Fixed ASAN test failures by removing .Times(1) and .Times(2) from
> CloseMultipleIdleSocketsHeldBy**LayeredPoolWhenNeeded unittest (this
> removes the
> tests relying on the order of std::set in CloseOneIdleConnectionInLayere**
> dPool).
> ASAN is prob
> ably causing the memory allocator to allocate the pools differently. The
> std::set is ordered by LayeredPool* which is the address of the LayeredPool
> (willchan).
>
> Added NET_EXPORT for layered_pool class defintion to fix windows shared
> compile.
>
> BUG=62364, 92244, 105839
> TEST=none
>
>
> Review URL:
http://codereview.chromium.**org/8340012<http://codereview.chromium.org/8340012>
>
> TBR=willchan@chromium.org
> Review URL:
http://codereview.chromium.**org/8745007<http://codereview.chromium.org/8745007>
>
> TBR=willchan@chromium.org
>
> Review URL:
http://codereview.chromium.**org/8803019<http://codereview.chromium.org/8803019>
>
> TBR=rtenneti@chromium.org
> Committed: http://src.chromium.org/**viewvc/chrome?view=rev&**
>
revision=113305<http://src.chromium.org/viewvc/chrome?view=rev&revision=113305>
>
> TBR=willchan@chromium.org
>
> Please review this at
http://codereview.chromium.**org/8836002/<http://codereview.chromium.org/8836...
>
> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>
> Affected files:
>  M     net/http/http_network_**transaction_unittest.cc
>  M     net/http/http_proxy_client_**socket_pool.h
>  M     net/http/http_proxy_client_**socket_pool.cc
>  M     net/socket/client_socket_**handle.h
>  M     net/socket/client_socket_**handle.cc
>  M     net/socket/client_socket_pool.**h
>  M     net/socket/client_socket_pool_**base.h
>  M     net/socket/client_socket_pool_**base.cc
>  M     net/socket/client_socket_pool_**base_unittest.cc
>  M     net/socket/socks_client_**socket_pool.h
>  M     net/socket/socks_client_**socket_pool.cc
>  M     net/socket/ssl_client_socket_**pool.h
>  M     net/socket/ssl_client_socket_**pool.cc
>  M     net/socket/transport_client_**socket_pool.h
>  M     net/socket/transport_client_**socket_pool.cc
>  M     net/spdy/spdy_session.h
>  M     net/spdy/spdy_session.cc
>
>
>

[willchan no longer on Chromium, 2011-12-07 03:40:56]

http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h
File net/socket/client_socket_pool.h (right):

http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h...
net/socket/client_socket_pool.h:34: class NET_EXPORT LayeredPool {
Wait, is this only used in tests? Perhaps it should be NET_EXPORT_PRIVATE?

[ramant (doing other things), 2011-12-07 03:47:52]

SpdySession and HttpProxyClientSocketPool use it.

http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h
File net/socket/client_socket_pool.h (right):

http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h...
net/socket/client_socket_pool.h:34: class NET_EXPORT LayeredPool {
On 2011/12/07 03:40:56, willchan wrote:
> Wait, is this only used in tests? Perhaps it should be NET_EXPORT_PRIVATE?

It is used by SpdySession, HttpProxyClientSocketPool

./http/http_proxy_client_socket_pool.h:171:    : public ClientSocketPool, public
LayeredPool {
./spdy/spdy_session.h:50:                               public LayeredPool {

[willchan no longer on Chromium, 2011-12-07 03:49:29]

Those are within the same library and don't need NET_EXPORT.

On Tue, Dec 6, 2011 at 7:47 PM, <rtenneti@chromium.org> wrote:

> SpdySession and HttpProxyClientSocketPool use it.
>
>
>
> http://codereview.chromium.**org/8836002/diff/1/net/socket/**
>
client_socket_pool.h<http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h>
> File net/socket/client_socket_pool.**h (right):
>
> http://codereview.chromium.**org/8836002/diff/1/net/socket/**
>
client_socket_pool.h#newcode34<http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h#newcode34>
> net/socket/client_socket_pool.**h:34: class NET_EXPORT LayeredPool {
> On 2011/12/07 03:40:56, willchan wrote:
>
>> Wait, is this only used in tests? Perhaps it should be
>>
> NET_EXPORT_PRIVATE?
>
> It is used by SpdySession, HttpProxyClientSocketPool
>
> ./http/http_proxy_client_**socket_pool.h:171:    : public
> ClientSocketPool, public LayeredPool {
> ./spdy/spdy_session.h:50:                               public
> LayeredPool {
>
>
http://codereview.chromium.**org/8836002/<http://codereview.chromium.org/8836...
>

[ramant (doing other things), 2011-12-07 03:57:37]

willchan and I had talked. We will go with NET_EXPORT and will talk to rvargas
about the right fix.

On 2011/12/07 03:49:29, willchan wrote:
> Those are within the same library and don't need NET_EXPORT.
> 
> On Tue, Dec 6, 2011 at 7:47 PM, <mailto:rtenneti@chromium.org> wrote:
> 
> > SpdySession and HttpProxyClientSocketPool use it.
> >
> >
> >
> > http://codereview.chromium.**org/8836002/diff/1/net/socket/**
> >
>
client_socket_pool.h<http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h>
> > File net/socket/client_socket_pool.**h (right):
> >
> > http://codereview.chromium.**org/8836002/diff/1/net/socket/**
> >
>
client_socket_pool.h#newcode34<http://codereview.chromium.org/8836002/diff/1/net/socket/client_socket_pool.h#newcode34>
> > net/socket/client_socket_pool.**h:34: class NET_EXPORT LayeredPool {
> > On 2011/12/07 03:40:56, willchan wrote:
> >
> >> Wait, is this only used in tests? Perhaps it should be
> >>
> > NET_EXPORT_PRIVATE?
> >
> > It is used by SpdySession, HttpProxyClientSocketPool
> >
> > ./http/http_proxy_client_**socket_pool.h:171:    : public
> > ClientSocketPool, public LayeredPool {
> > ./spdy/spdy_session.h:50:                               public
> > LayeredPool {
> >
> >
>
http://codereview.chromium.**org/8836002/%3Chttp://codereview.chromium.org/88...>
> >