Close idle connections / SPDY sessions when needed.

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. 

BUG=62364, 92244
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112130

[willchan no longer on Chromium, 2011-10-26 23:54:30]

This is probably a difficult change to review. The connection pool stuff is very
confusing and their interaction is very complicated. I'm hoping to get others
ramped up on this now that mbelshe is gone. Please feel free to ask me a lot of
questions about how this works.

[ramant (doing other things), 2011-10-27 17:33:56]

hi willchan,
  Changes LGTM (I am not 100% sure if I have the authority to review this code,
but I couldn't find any mistakes).

thanks,
raman

[mmenke, 2011-10-27 18:52:37]

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_han...
File net/socket/client_socket_handle.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_han...
net/socket/client_socket_handle.cc:85: void
ClientSocketHandle::AddLayeredPool(LayeredPool* layered_pool) {
DCHECK(!layered_pool), maybe?

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:354: } else if
(!CloseOneIdleConnectionInLayeredPool()) {
Is this guaranteed to result in ReachedMaxSocketsLimit() returning false?  Seems
to me that if we had a socket with |ignore_limits| set to true, maybe not.

May be an obscure enough case that we can ignore it here, though, as things
should still work.

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:890: CHECK_LE(handed_out_socket_count_ +
connecting_socket_count_, max_sockets_);
Given that requests have an "ignore_limits" flag, I think this CHECK may result
in random crashing whenever it's set.

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:1073: higher_layer_pools_.begin(); it !=
higher_layer_pools_.end(); ++it) {
nit:  Looks to me like higher_layer_pools_.begin(); will just fit on the
previous line, making for more readable code.

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base_unittest.cc:3353: EXPECT_EQ(OK,
mock_layered_pool.RequestSocket(pool_.get()));
Don't think you need to request a socket twice in this test.

http://codereview.chromium.org/8340012/diff/2001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_pool.cc:618: bool
SSLClientSocketPool::CloseOneIdleConnection() {
Since you duplicate this code 3 or 4 times, and never use the underlying two
SocketPoolBase functions independently, you might want to just move this code up
into SocketPoolBaseHelper.

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:974: RemoveFromPool();
Is this safe?  It looks to me like there may be no one holding a reference to
|this| when we hit the line "spdy_session_pool_ = NULL;" in RemoveFromPool.

[willchan no longer on Chromium, 2011-11-08 22:26:13]

Finally got back to this CL.

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:354: } else if
(!CloseOneIdleConnectionInLayeredPool()) {
On 2011/10/27 18:52:37, Matt Menke wrote:
> Is this guaranteed to result in ReachedMaxSocketsLimit() returning false? 
Seems
> to me that if we had a socket with |ignore_limits| set to true, maybe not.
> 
> May be an obscure enough case that we can ignore it here, though, as things
> should still work.

Sorry, I don't grok. Which path do you mean? Also note that we're within a code
block where !request->ignore_limits().

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:890: CHECK_LE(handed_out_socket_count_ +
connecting_socket_count_, max_sockets_);
On 2011/10/27 18:52:37, Matt Menke wrote:
> Given that requests have an "ignore_limits" flag, I think this CHECK may
result
> in random crashing whenever it's set.

Good point!

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:1073: higher_layer_pools_.begin(); it !=
higher_layer_pools_.end(); ++it) {
On 2011/10/27 18:52:37, Matt Menke wrote:
> nit:  Looks to me like higher_layer_pools_.begin(); will just fit on the
> previous line, making for more readable code.

Done.

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base_unittest.cc:3353: EXPECT_EQ(OK,
mock_layered_pool.RequestSocket(pool_.get()));
On 2011/10/27 18:52:37, Matt Menke wrote:
> Don't think you need to request a socket twice in this test.

Done.

http://codereview.chromium.org/8340012/diff/2001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_pool.cc:618: bool
SSLClientSocketPool::CloseOneIdleConnection() {
On 2011/10/27 18:52:37, Matt Menke wrote:
> Since you duplicate this code 3 or 4 times, and never use the underlying two
> SocketPoolBase functions independently, you might want to just move this code
up
> into SocketPoolBaseHelper.

Since it's just 3 lines done in 3 places, I'm going to hold off on this, since
it's not inconceivable to have multiple ClientSocketPoolBaseHelpers.

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:974: RemoveFromPool();
On 2011/10/27 18:52:37, Matt Menke wrote:
> Is this safe?  It looks to me like there may be no one holding a reference to
> |this| when we hit the line "spdy_session_pool_ = NULL;" in RemoveFromPool.

I'm not sure I understand the worry. When we call RemoveFromPool() here, it may
indeed delete |this|, but since we just return right after, we don't dereference
|this|, so it should be safe. Or does the overall pattern just make you nervous?

[mmenke, 2011-11-08 22:41:03]

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:354: } else if
(!CloseOneIdleConnectionInLayeredPool()) {
On 2011/11/08 22:26:13, willchan wrote:
> On 2011/10/27 18:52:37, Matt Menke wrote:
> > Is this guaranteed to result in ReachedMaxSocketsLimit() returning false? 
> Seems
> > to me that if we had a socket with |ignore_limits| set to true, maybe not.
> > 
> > May be an obscure enough case that we can ignore it here, though, as things
> > should still work.
> 
> Sorry, I don't grok. Which path do you mean? Also note that we're within a
code
> block where !request->ignore_limits().

So you have a request with ignore_limits() set to true.  We then exceed the
limit by one.  A new request then reaches this code.  We close an idle socket. 
We're now at the limit.  We create a new socket, and end up exceeding the limit
once more.

Not exactly a critical problem, but does manage to bypass the limit in a rather
non-transparent manner.

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:974: RemoveFromPool();
On 2011/11/08 22:26:13, willchan wrote:
> On 2011/10/27 18:52:37, Matt Menke wrote:
> > Is this safe?  It looks to me like there may be no one holding a reference
to
> > |this| when we hit the line "spdy_session_pool_ = NULL;" in RemoveFromPool.
> 
> I'm not sure I understand the worry. When we call RemoveFromPool() here, it
may
> indeed delete |this|, but since we just return right after, we don't
dereference
> |this|, so it should be safe. Or does the overall pattern just make you
nervous?

We don't dereference |this|, but RemoveFromPool does, right after calling
spdy_session_pool_->Remove() on |this|.  Not clear anyone's holding a reference
to |this| when we reach the "spdy_session_pool_ = NULL;" line.

[willchan no longer on Chromium, 2011-11-15 18:44:06]

Ugh, I'm blocked on this changelist by some tests breaking because now I'm
introducing tight dependencies between socket pools, and previously we were able
to insert mock pools at whatever level we wanted via a peer.

My solution is to turn ClientSocketPoolManager into an interface so I can do
true dependency injection. I'm going to do that in a separate changelist.

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:354: } else if
(!CloseOneIdleConnectionInLayeredPool()) {
On 2011/11/08 22:41:04, Matt Menke wrote:
> On 2011/11/08 22:26:13, willchan wrote:
> > On 2011/10/27 18:52:37, Matt Menke wrote:
> > > Is this guaranteed to result in ReachedMaxSocketsLimit() returning false? 
> > Seems
> > > to me that if we had a socket with |ignore_limits| set to true, maybe not.
> > > 
> > > May be an obscure enough case that we can ignore it here, though, as
things
> > > should still work.
> > 
> > Sorry, I don't grok. Which path do you mean? Also note that we're within a
> code
> > block where !request->ignore_limits().
> 
> So you have a request with ignore_limits() set to true.  We then exceed the
> limit by one.  A new request then reaches this code.  We close an idle socket.

> We're now at the limit.  We create a new socket, and end up exceeding the
limit
> once more.
> 
> Not exactly a critical problem, but does manage to bypass the limit in a
rather
> non-transparent manner.

GREAT catch. I'm impressed you caught this given your relative unfamiliarity
with this case. I'll see if I can write a test to catch this behavior.

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/8340012/diff/2001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:974: RemoveFromPool();
On 2011/11/08 22:41:04, Matt Menke wrote:
> On 2011/11/08 22:26:13, willchan wrote:
> > On 2011/10/27 18:52:37, Matt Menke wrote:
> > > Is this safe?  It looks to me like there may be no one holding a reference
> to
> > > |this| when we hit the line "spdy_session_pool_ = NULL;" in
RemoveFromPool.
> > 
> > I'm not sure I understand the worry. When we call RemoveFromPool() here, it
> may
> > indeed delete |this|, but since we just return right after, we don't
> dereference
> > |this|, so it should be safe. Or does the overall pattern just make you
> nervous?
> 
> We don't dereference |this|, but RemoveFromPool does, right after calling
> spdy_session_pool_->Remove() on |this|.  Not clear anyone's holding a
reference
> to |this| when we reach the "spdy_session_pool_ = NULL;" line.

Again, great catch. I've fixed this.

[mmenke, 2011-11-15 19:00:23]

Looks like you didn't upload a new patch set.  I'm not sure if that's
deliberate, pending the new CL you mentioned, or not.

http://codereview.chromium.org/8340012/diff/6001/net/socket/client_socket_han...
File net/socket/client_socket_handle.cc (right):

http://codereview.chromium.org/8340012/diff/6001/net/socket/client_socket_han...
net/socket/client_socket_handle.cc:86: CHECK(layered_pool);
I was actually thinking CHECK(!layered_pool_).

http://codereview.chromium.org/8340012/diff/10001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/10001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9271: <<<<<<< HEAD
Looks like you forgot to remove this.

[willchan no longer on Chromium, 2011-11-15 19:03:30]

Yeah, still pending the new CL. Sorry.

On Tue, Nov 15, 2011 at 2:00 PM, <mmenke@chromium.org> wrote:

> Looks like you didn't upload a new patch set.  I'm not sure if that's
> deliberate, pending the new CL you mentioned, or not.
>
>
> http://codereview.chromium.**org/8340012/diff/6001/net/**
>
socket/client_socket_handle.cc<http://codereview.chromium.org/8340012/diff/6001/net/socket/client_socket_handle.cc>
> File net/socket/client_socket_**handle.cc (right):
>
> http://codereview.chromium.**org/8340012/diff/6001/net/**
>
socket/client_socket_handle.**cc#newcode86<http://codereview.chromium.org/8340012/diff/6001/net/socket/client_socket_handle.cc#newcode86>
> net/socket/client_socket_**handle.cc:86: CHECK(layered_pool);
> I was actually thinking CHECK(!layered_pool_).
>
> http://codereview.chromium.**org/8340012/diff/10001/net/**
>
http/http_network_transaction_**unittest.cc<http://codereview.chromium.org/8340012/diff/10001/net/http/http_network_transaction_unittest.cc>
> File net/http/http_network_**transaction_unittest.cc (right):
>
> http://codereview.chromium.**org/8340012/diff/10001/net/**
>
http/http_network_transaction_**unittest.cc#newcode9271<http://codereview.chromium.org/8340012/diff/10001/net/http/http_network_transaction_unittest.cc#newcode9271>
> net/http/http_network_**transaction_unittest.cc:9271: <<<<<<< HEAD
> Looks like you forgot to remove this.
>
>
http://codereview.chromium.**org/8340012/<http://codereview.chromium.org/8340...
>

[willchan no longer on Chromium, 2011-11-25 22:22:10]

I think this is working now. I have to fiddle with a timeout for the Windows
trybot, but the Mac errors aren't mine, so this is 99.9% ready.

[mmenke, 2011-11-28 17:47:13]

LGTM.  Just a number of nits.

http://codereview.chromium.org/8340012/diff/19001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9722: MockRead("hello!"),
nit:  Think using something other than "hello!" would make this test a little
clearer.  Was at first confused why this was appearing on the first SPDY socket,
until I realized it was identical to the body of the SPDY responses, hard-coded
elsewhere.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_ha...
File net/socket/client_socket_handle.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_ha...
net/socket/client_socket_handle.cc:86: CHECK(layered_pool);
nit:  Still think we should also have a CHECK(!layered_pool_) here, too.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:375: return ERR_IO_PENDING;
nit:  Indent 2 more spaces.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:321: // I'm not sure if we hit this
situation often.
Random comment:  Since we hit this situation often enough with proxies for this
CL to be worth it, we may well hit this situation enough in proxy situations to
be smarter about which socket we close.  Would say it's low priority,
regardless, though.  And yea, I know you just copied the comment.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:341: bool
CloseOneIdleConnectionInLayeredPool();
nit:  No need to reorder everything to be consistent, but this function and
CloseOneIdleSocket are declared right next ro each other in
ClientSocketPoolBase, so it seems logical to have them right next to each other
here as well, and the same for their bodies in the cc file.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:420: void
AddLayeredPool(LayeredPool* pool) {
virtual/OVERRIDE

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:420: void
AddLayeredPool(LayeredPool* pool) {
nit:  These two should go in the same place they appear in client_socket_pool.h,
which currently is between GetLoadState and GetInfoAsValue.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:424: void
RemoveLayeredPool(LayeredPool* pool) {
virtual/OVERRIDE

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:434: const BoundNetLog& net_log)
{
nit:  OVERRIDE

http://codereview.chromium.org/8340012/diff/19001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/8340012/diff/19001/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:17: #include "base/memory/weak_ptr.h"
nit:  Alphabetize

[willchan no longer on Chromium, 2011-11-29 23:14:52]

http://codereview.chromium.org/8340012/diff/19001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:9722: MockRead("hello!"),
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  Think using something other than "hello!" would make this test a little
> clearer.  Was at first confused why this was appearing on the first SPDY
socket,
> until I realized it was identical to the body of the SPDY responses,
hard-coded
> elsewhere.

Yeah, it's hardcoded elsewhere. It's probably too much trouble to fix right
here.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_ha...
File net/socket/client_socket_handle.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_ha...
net/socket/client_socket_handle.cc:86: CHECK(layered_pool);
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  Still think we should also have a CHECK(!layered_pool_) here, too.

Done.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:375: return ERR_IO_PENDING;
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  Indent 2 more spaces.

Done.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:321: // I'm not sure if we hit this
situation often.
On 2011/11/28 17:47:14, Matt Menke wrote:
> Random comment:  Since we hit this situation often enough with proxies for
this
> CL to be worth it, we may well hit this situation enough in proxy situations
to
> be smarter about which socket we close.  Would say it's low priority,
> regardless, though.  And yea, I know you just copied the comment.

You're probably right. I'll file a bug.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:341: bool
CloseOneIdleConnectionInLayeredPool();
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  No need to reorder everything to be consistent, but this function and
> CloseOneIdleSocket are declared right next ro each other in
> ClientSocketPoolBase, so it seems logical to have them right next to each
other
> here as well, and the same for their bodies in the cc file.

Done.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:420: void
AddLayeredPool(LayeredPool* pool) {
On 2011/11/28 17:47:14, Matt Menke wrote:
> virtual/OVERRIDE

Done.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:420: void
AddLayeredPool(LayeredPool* pool) {
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  These two should go in the same place they appear in
client_socket_pool.h,
> which currently is between GetLoadState and GetInfoAsValue.

Done.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:424: void
RemoveLayeredPool(LayeredPool* pool) {
On 2011/11/28 17:47:14, Matt Menke wrote:
> virtual/OVERRIDE

Done.

http://codereview.chromium.org/8340012/diff/19001/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:434: const BoundNetLog& net_log)
{
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  OVERRIDE

Done.

http://codereview.chromium.org/8340012/diff/19001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/8340012/diff/19001/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:17: #include "base/memory/weak_ptr.h"
On 2011/11/28 17:47:14, Matt Menke wrote:
> nit:  Alphabetize

Done.

[commit-bot: I haz the power, 2011-11-29 23:51:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/8340012/31001

[commit-bot: I haz the power, 2011-11-30 01:17:38]

Try job failure for 8340012-31001 (previous was lost) (retry) (previous was
lost) on mac_rel for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-30 02:24:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/8340012/35001

[commit-bot: I haz the power, 2011-11-30 04:57:28]

Change committed as 112130