If an idle socket has received data unexpectedly, we...

If an idle socket has received data unexpectedly, we can't
reuse it.

Add the IsConnectedAndIdle method, which returns true if the
connection is still alive and idle (hasn't received any data
unexpectedly).

R=eroman
BUG=4606
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=10060

[wtc, 2009-02-19 03:38:55]

eroman: please review this CL.

darin: just FYI.  To fix this bug, I have to change the
definition of IsConnected.  To match the new definition,
I rename the method CanReuse.  You may want to look at
just the three places I noted below.

http://codereview.chromium.org/21501/diff/1/10
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/21501/diff/1/10#newcode95
Line 95: EXPECT_TRUE(sock->CanReuse());
This is an example of a place where the new name "CanReuse"
sounds strange, because we definitely aren't trying to reuse
the socket here.

http://codereview.chromium.org/21501/diff/1/14
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/21501/diff/1/14#newcode157
Line 157: if (rv >= 0)
This is the actual bug fix (for Mac/Linux).

http://codereview.chromium.org/21501/diff/1/12
File net/base/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/21501/diff/1/12#newcode153
Line 153: if (rv >= 0)
This is the actual bug fix (for Windows).

[eroman, 2009-02-19 04:44:18]

lgtm

http://codereview.chromium.org/21501/diff/1/17
File net/base/client_socket.h (right):

http://codereview.chromium.org/21501/diff/1/17#newcode51
Line 51: virtual bool CanReuse() const = 0;
I wander if there is a name which captures the state of the socket, as opposed
to the policy we apply to it?

Some other ideas:

  IsConnectedAndEmpty()
  IsConnectedAndIdle()
  IsConnectedAndNoUnreadData()

(somehow my function names always end up being a mile long :-)

http://codereview.chromium.org/21501/diff/1/16
File net/base/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/21501/diff/1/16#newcode301
Line 301: // Ideally, we should also check if we have received the close_notify
alert
Does this comment need to be updated?
Seems if we had received a close_notify, then there would be unread bytes in the
socket, and CanReuse() returns false.

[wtc, 2009-02-19 19:55:52]

Please review Patch Set 2.  I agree it's bad to name the
method with a policy specific to HTTP keep-alive
connections, but other than the unit tests, we only use
that method in that context.  If the CanReuse name becomes
confusing in some new code, we can rename it then.

Ideally, we want two methods:
1. IsConnected
2. HasReceivedData

The problem is that each method requires a system call,
but the same system call can also tell us
IsConnected() && !HasReceivedData(), so for performance
we also want a method for that combo.  It turns out
we only use that combo, so that's why I only kept the
combo method (CanReuse).

[wtc, 2009-02-19 22:24:40]

Darin: just FYI: as we discussed, I kept IsConnected and
added IsConnectedAndIdle.

Eric: please review Patch Set 4.

[eroman, 2009-02-19 22:48:25]

lgtm

http://codereview.chromium.org/21501/diff/60/76
File net/base/client_socket.h (right):

http://codereview.chromium.org/21501/diff/60/76#newcode52
Line 52: // have been received.
"have been" --> "has been"