Get rid of the data socket completely when data transfer is finished.

Get rid of the data socket completely when data transfer is finished.

Fixes an FTP transaction hang when we wait for server's response
and the server apparently waits for us.

TEST=See bug.
http://crbug.com/21127

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

[Paweł Hajdan Jr., 2009-09-08 19:14:38]

CC willchan - I don't quite understand why it's necessary - the data socket
should be disconnected at this point.

I'm not sure what difference destroying it makes, but it fixes the problem in
this case.

I was testing this on Linux, so it was using tcp_client_socket_libevent variant.

[willchan no longer on Chromium, 2009-09-08 20:09:17]

Why are you sure the socket is disconnected?  !IsConnected() does not imply that
it's already been close()'d.  We could be waiting on a connect(), or recv() w/
MSG_PEEK might indicate that the peer has shutdown() its side of the connection,
but we have not shutdown() our side.

On 2009/09/08 19:14:38, Paweł Hajdan Jr. wrote:
> CC willchan - I don't quite understand why it's necessary - the data socket
> should be disconnected at this point.
> 
> I'm not sure what difference destroying it makes, but it fixes the problem in
> this case.
> 
> I was testing this on Linux, so it was using tcp_client_socket_libevent
variant.

[Paweł Hajdan Jr., 2009-09-08 20:10:30]

On 2009/09/08 20:09:17, willchan wrote:
> Why are you sure the socket is disconnected?

Well, I wasn't sure. After your explanation I'm a bit more confident about this
fix.

[willchan no longer on Chromium, 2009-09-08 20:18:56]

On 2009/09/08 20:10:30, Paweł Hajdan Jr. wrote:
> On 2009/09/08 20:09:17, willchan wrote:
> > Why are you sure the socket is disconnected?
> 
> Well, I wasn't sure. After your explanation I'm a bit more confident about
this
> fix.

Ok.  You can test my explanation by calling Disconnect() instead of deleting the
socket object.  Disconnect() won't do anything if the socket has already been
closed, otherwise it will close() it.  Therefore, it should do roughly the same
thing as reset().

That said, the semantics of the IsConnected() method seems potentially confusing
to me.  I wonder if there are any other bugs like this in our codebase.  Food
for thought.

In any case, the current fix LGTM if it fixes the issues you're seeing.

[wtc, 2009-09-09 20:20:42]

LGTM.

http://codereview.chromium.org/192042/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/192042/diff/1/2#newcode984
Line 984: // If we don't destroy the data socket completely, some servers will
wait
It would be nice to explain that a half-closed TCP connection
still needs to be closed on our side.