Cancel any outstanding host resolve when calling SOCKSClientSocket::Disconnec...

Cancel any outstanding host resolve when calling SOCKSClientSocket::Disconnect().

BUG=25440
TEST=SOCKSClientSocketTest.DisconnectWhileHostResolveInProgress


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

[wtc, 2010-01-07 18:55:06]

LGTM.

http://codereview.chromium.org/525084/diff/1/5
File net/base/host_resolver.h (right):

http://codereview.chromium.org/525084/diff/1/5#newcode189
net/base/host_resolver.h:189: // from being invoked.
Does this allow us to call Resolve() again?  The class's
name "SingleRequestHostResolver" seems to suggest that you
can use this object only once.  If I misunderstood that,
please document this.

http://codereview.chromium.org/525084/diff/1/2
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/525084/diff/1/2#newcode98
net/socket/socks5_client_socket.cc:98: // Reset other states to make sure they
aren't mistakenly used later.
To help future maintainers of this code, please explain
how you chose the class members that need to be reset,
among all the members of the class.
I believe these are the arguments of the Connect() method.

http://codereview.chromium.org/525084/diff/1/2#newcode102
net/socket/socks5_client_socket.cc:102: load_log_ = NULL;
Should we add a message to the load_log_ before setting
it to NULL?

http://codereview.chromium.org/525084/diff/1/3
File net/socket/socks_client_socket_unittest.cc (right):

http://codereview.chromium.org/525084/diff/1/3#newcode325
net/socket/socks_client_socket_unittest.cc:325: class HangingHostResolver :
public HostResolver {
Can you move this class definition to the top of the file?
It looks nicer that way (I expect to see class definitions
at the beginning of the file).

http://codereview.chromium.org/525084/diff/1/3#newcode335
net/socket/socks_client_socket_unittest.cc:335: outstanding_request_ =
reinterpret_cast<RequestHandle> (1);
Nit: delete the space before "(1)".

http://codereview.chromium.org/525084/diff/1/3#newcode337
net/socket/socks_client_socket_unittest.cc:337:
EXPECT_TRUE(HasOutstandingRequest());
This EXPECT_TRUE is not useful because the condition is
clearly true by code inspection.

http://codereview.chromium.org/525084/diff/1/3#newcode345
net/socket/socks_client_socket_unittest.cc:345:
EXPECT_FALSE(HasOutstandingRequest());
Same here: this EXPECT_FALSE is not useful because the
condition is clearly false by code inspection.

http://codereview.chromium.org/525084/diff/1/3#newcode366
net/socket/socks_client_socket_unittest.cc:366: 
Nit: delete this blank line.

[eroman, 2010-01-07 19:35:43]

Thanks for the review!

http://codereview.chromium.org/525084/diff/1/5
File net/base/host_resolver.h (right):

http://codereview.chromium.org/525084/diff/1/5#newcode189
net/base/host_resolver.h:189: // from being invoked.
On 2010/01/07 18:55:06, wtc wrote:
> Does this allow us to call Resolve() again?  The class's
> name "SingleRequestHostResolver" seems to suggest that you
> can use this object only once.  If I misunderstood that,
> please document this.

Yes, Resolve() can be called again after this.
I added a comment to Cancel()'s documentation confirming this.

SingleRequestHostResolver limits us to having 0 or 1 resolve in flight at a
time, but can be used for multiple resolves. I could elaborate this in the
header documentation, which currently reads:

// It wraps HostResolver to resolve only a single hostname at a time and cancels
// this request when going out of scope.

http://codereview.chromium.org/525084/diff/1/2
File net/socket/socks5_client_socket.cc (right):

http://codereview.chromium.org/525084/diff/1/2#newcode98
net/socket/socks5_client_socket.cc:98: // Reset other states to make sure they
aren't mistakenly used later.
On 2010/01/07 18:55:06, wtc wrote:
> To help future maintainers of this code, please explain
> how you chose the class members that need to be reset,
> among all the members of the class.
> I believe these are the arguments of the Connect() method.

Done.

http://codereview.chromium.org/525084/diff/1/3
File net/socket/socks_client_socket_unittest.cc (right):

http://codereview.chromium.org/525084/diff/1/3#newcode325
net/socket/socks_client_socket_unittest.cc:325: class HangingHostResolver :
public HostResolver {
On 2010/01/07 18:55:06, wtc wrote:
> Can you move this class definition to the top of the file?
> It looks nicer that way (I expect to see class definitions
> at the beginning of the file).

Done.

http://codereview.chromium.org/525084/diff/1/3#newcode335
net/socket/socks_client_socket_unittest.cc:335: outstanding_request_ =
reinterpret_cast<RequestHandle> (1);
On 2010/01/07 18:55:06, wtc wrote:
> Nit: delete the space before "(1)".

Done.

http://codereview.chromium.org/525084/diff/1/3#newcode337
net/socket/socks_client_socket_unittest.cc:337:
EXPECT_TRUE(HasOutstandingRequest());
On 2010/01/07 18:55:06, wtc wrote:
> This EXPECT_TRUE is not useful because the condition is
> clearly true by code inspection.

Done.

(These were here because I was debugging a problem with the unit-test during
development).

http://codereview.chromium.org/525084/diff/1/3#newcode345
net/socket/socks_client_socket_unittest.cc:345:
EXPECT_FALSE(HasOutstandingRequest());
On 2010/01/07 18:55:06, wtc wrote:
> Same here: this EXPECT_FALSE is not useful because the
> condition is clearly false by code inspection.

Done.

http://codereview.chromium.org/525084/diff/1/3#newcode366
net/socket/socks_client_socket_unittest.cc:366: 
On 2010/01/07 18:55:06, wtc wrote:
> Nit: delete this blank line.

Done.

[wtc, 2010-01-07 19:45:14]

LGTM.

Do you think it's worth logging the fact that the
Connect was cancelled by Disconnect before we set
load_log_ to NULL?

[eroman, 2010-01-07 20:00:52]

> Do you think it's worth logging the fact that the
>  Connect was cancelled by Disconnect before we set
> load_log_ to NULL?

Yes, I think it is worth doing since it doesn't really cost us anything.
But per my previous comment, I don't think such change should be part of this
CL.