POSIX only: Move client socket functionality from TCPClientSocket into TCPSocket.

POSIX only: Move client socket functionality from TCPClientSocket into TCPSocket.

TCPClientSocket becomes a wrapper around TCPSocket to expose a client-only interface.

BUG=262601
TEST=tcp_socket_unittest.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223945

[yzshen1, 2013-09-05 21:32:33]

Hi, Fred and Wan-Teh.

Would you please take a look? Thanks!

[akalin, 2013-09-05 22:58:12]

few nits, will wait for other CL to land and then I'll review again

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_server_sock...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_server_sock...
net/socket/tcp_server_socket.cc:93: *output_accepted_socket =
client_socket.Pass();
you can do:

output_accepted_socket->reset(new ...);

now

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
net/socket/tcp_socket_libevent.cc:118: write_ready_callback_.Run();
assuming moved code here, too

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
net/socket/tcp_socket_libevent.h:24: class AddressList;
i'm assuming this is all moved from tcp_client_socket_libevent.h

[yzshen1, 2013-09-06 17:19:44]

Thanks Fred!

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_server_sock...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_server_sock...
net/socket/tcp_server_socket.cc:93: *output_accepted_socket =
client_socket.Pass();
On 2013/09/05 22:58:12, akalin wrote:
> you can do:
> 
> output_accepted_socket->reset(new ...);
> 
> now

Done. Looks much better. :)

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
net/socket/tcp_socket_libevent.cc:118: write_ready_callback_.Run();
On 2013/09/05 22:58:12, akalin wrote:
> assuming moved code here, too

Please see my reply for your comment in the .h file.

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/23454010/diff/3002/net/socket/tcp_socket_libe...
net/socket/tcp_socket_libevent.h:24: class AddressList;
Mostly yes. A few major changes include:
- ReadWatcher/WriteWatcher are replaced with a more generic Watcher class, which
also supports accept.
- the mutiple-connect-attempts logic is split out and lives in
tcp_client_socket. (Therefore, maybe you want to look at those *Connect*()
methods.)
- the UseHistory logging logic is also split out and lives in tcp_client_socket.
- Close() is made to reset most of the internal state (except logging), so that
it is okay to reuse the object and call Create() again.

On 2013/09/05 22:58:12, akalin wrote:
> i'm assuming this is all moved from tcp_client_socket_libevent.h

[yzshen1, 2013-09-13 06:56:18]

Hi, Wan-Teh and Fred.

I have updated the code and addressed issues that you raised for the Windows CL.
Please take another look.

Thanks!

[wtc, 2013-09-13 22:22:44]

Patch set 6 LGTM.

Did someone suggest the use of Closure in the Watcher class?
That is one big change to the original code. This change is
fine by me. I am just curious where it comes from.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket.cc#...
net/socket/tcp_socket.cc:47: }

Add a "// namespace" comment.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket.cc#...
net/socket/tcp_socket.cc:49: static bool g_tcp_fastopen_enabled = false;

Move this inside the unnamed namespace and remove "static".

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:5: #include "net/socket/tcp_socket.h"

This should include tcp_socket_libevent.h because this file is named
tcp_socket_libevent.cc.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:16: #include <netinet/in.h>

I believe this file can only be used on OS_POSIX. Some of the C system headers
we include above only exist on OS_POSIX. So this #if defined(OS_POSIX) check
seems unnecessary.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:113: read_ready_callback_.Run();

Do we need to reset read_ready_callback_ and write_ready_callback_ after
running them?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:158: Close();

It may be better to call Close() before logging events and updating histograms.
The original code does this.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:269: // completed and failed. Connecting the
same |socket_| again after a

Nit: "Please note that we enforce this ... and failed." can be removed because
it applies only to Windows.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:298: if (use_tcp_fastopen_ &&
!tcp_fastopen_connected_ && peer_address_) {

Do we need to check peer_address_ in this conditional expression?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:417: DCHECK(address);

We probably should do
  if (socket_ == kInvalidSocket)
    return ERR_SOCKET_NOT_CONNECTED;
to match the original code.

Note: it is possible that the getsockname call will fail with the same error.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:423: return ERR_FAILED;

Please use a better error code. (You fixed this in tcp_socket_win.cc.)

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:586: return ERR_FAILED;

Use a better error code (on lines 585 and 586).

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:690: if (read_callback_.is_null())

Do you know why read_callback_ can be null? (I know this is_null() check
comes from the original code.)

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:762: if (os_error == EINPROGRESS || os_error
== EALREADY) {

Let's remove this check and the TODO comment.

EINPROGRESS and EALREADY can only be returned by connect(). So we won't get
these two error codes from the getsockopt(...SO_ERROR...) call above.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:781: } else if (!write_callback_.is_null()) {

(I know this is_null() check comes from the original code.)

I suggest we just call DidCompleteWrite, and let DidCompleteWrite check
write_callback_.is_null() just like DidCompleteRead checks
read_callback_.is_null().

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:192: Watcher write_watcher_;

Since Watcher has both a read_ready_callback_ and a write_ready_callback_,
it seems that we need just one Watcher object for both Read and Write.
Will that work?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:198: int write_buf_len_;

Should we keep the original comments for these members (from
read_socket_watcher_
to write_buf_len_)?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:212: enum FastOpenStatus fast_open_status_;

I think in C++ we don't need to say "enum" here.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:214: // A connect operation is pending. In this
case, |write_callback_| needs to be

In tcp_socket_win.h we use the read_callback_ for connect. Would be nice to
be consistent. (I think it is better to use the write callback.)

[yzshen1, 2013-09-13 23:52:40]

Thanks Wan-Teh!

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket.cc
File net/socket/tcp_socket.cc (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket.cc#...
net/socket/tcp_socket.cc:47: }
On 2013/09/13 22:22:44, wtc wrote:
> 
> Add a "// namespace" comment.

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket.cc#...
net/socket/tcp_socket.cc:49: static bool g_tcp_fastopen_enabled = false;
On 2013/09/13 22:22:44, wtc wrote:
> 
> Move this inside the unnamed namespace and remove "static".

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:5: #include "net/socket/tcp_socket.h"
This is intended, because we need IsTCPFastOpenEnabled() from tcp_socket.h.

The previous code tcp_client_socket_libevent.cc included tcp_client_socket.h for
the same reason.


On 2013/09/13 22:22:44, wtc wrote:
> 
> This should include tcp_socket_libevent.h because this file is named
> tcp_socket_libevent.cc.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:16: #include <netinet/in.h>
On 2013/09/13 22:22:44, wtc wrote:
> 
> I believe this file can only be used on OS_POSIX. Some of the C system headers
> we include above only exist on OS_POSIX. So this #if defined(OS_POSIX) check
> seems unnecessary.

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:113: read_ready_callback_.Run();
On 2013/09/13 22:22:44, wtc wrote:
> 
> Do we need to reset read_ready_callback_ and write_ready_callback_ after
> running them?

|read_ready_callback_| here is a closure to forward the notification to a
certain method of TCPSocketLibevent. It never changes during the lifetime of
TCPSocketLibevent. So we shouldn't reset it. (same for |write_ready_callback_|.)

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:158: Close();
That will reset |tcp_fastopen_connected_| and cause the previous if-block to
never run.

On 2013/09/13 22:22:44, wtc wrote:
> 
> It may be better to call Close() before logging events and updating
histograms.
> The original code does this.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:269: // completed and failed. Connecting the
same |socket_| again after a
On 2013/09/13 22:22:44, wtc wrote:
> 
> Nit: "Please note that we enforce this ... and failed." can be removed because
> it applies only to Windows.

I thought this sentence also applies here? 
Maybe I have misunderstood something?

I changed it to "Please note that we don't allow a second Connect() even if the
previous Connect() has failed". Does it sound better?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:298: if (use_tcp_fastopen_ &&
!tcp_fastopen_connected_ && peer_address_) {
Yes. The following is one example:

- |use_tcp_fastopen_| is always true if fast open is enabled.
- the user hasn't called Connect(), |tcp_fastopen_connected_| == false.

In this case, we don't want IsConnected() to return true.


On 2013/09/13 22:22:44, wtc wrote:
> 
> Do we need to check peer_address_ in this conditional expression?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:417: DCHECK(address);
IMHO, ERR_SOCKET_NOT_CONNECTED is not accurate.
For example, GetLocalAddress() can be used after Bind() but before Connect().
It is not a method related to the connection status.

Removing this part matches the behavior of tcp_socket_win.cc as well. What do
you think?


On 2013/09/13 22:22:44, wtc wrote:
> 
> We probably should do
>   if (socket_ == kInvalidSocket)
>     return ERR_SOCKET_NOT_CONNECTED;
> to match the original code.
> 
> Note: it is possible that the getsockname call will fail with the same error.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:423: return ERR_FAILED;
On 2013/09/13 22:22:44, wtc wrote:
> 
> Please use a better error code. (You fixed this in tcp_socket_win.cc.)

Done. I used ERR_ADDRESS_INVALID.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:586: return ERR_FAILED;
I changed to ERR_ADDRESS_INVALID. Does it sound okay for accept to return
ERR_ADDRESS_INVALID?

On 2013/09/13 22:22:44, wtc wrote:
> 
> Use a better error code (on lines 585 and 586).

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:690: if (read_callback_.is_null())
I don't know any scenario in which read_callback_ can be null when
DidCompleteRead() is called.
I just migrated it from the original code.

Do you suggest me to remove it or we just stay at the safe side for now?

On 2013/09/13 22:22:44, wtc wrote:
> 
> Do you know why read_callback_ can be null? (I know this is_null() check
> comes from the original code.)

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:762: if (os_error == EINPROGRESS || os_error
== EALREADY) {
On 2013/09/13 22:22:44, wtc wrote:
> 
> Let's remove this check and the TODO comment.
> 
> EINPROGRESS and EALREADY can only be returned by connect(). So we won't get
> these two error codes from the getsockopt(...SO_ERROR...) call above.

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:781: } else if (!write_callback_.is_null()) {
On 2013/09/13 22:22:44, wtc wrote:
> 
> (I know this is_null() check comes from the original code.)
> 
> I suggest we just call DidCompleteWrite, and let DidCompleteWrite check
> write_callback_.is_null() just like DidCompleteRead checks
> read_callback_.is_null().

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:192: Watcher write_watcher_;
I slightly prefer to have one Watcher object for each FileDescriptorWatcher.
That is also what the original code did. What do you think?

On 2013/09/13 22:22:44, wtc wrote:
> 
> Since Watcher has both a read_ready_callback_ and a write_ready_callback_,
> it seems that we need just one Watcher object for both Read and Write.
> Will that work?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:198: int write_buf_len_;
On 2013/09/13 22:22:44, wtc wrote:
> 
> Should we keep the original comments for these members (from
> read_socket_watcher_
> to write_buf_len_)?

Done. Some of the comments were outdated and I updated them.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:212: enum FastOpenStatus fast_open_status_;
On 2013/09/13 22:22:44, wtc wrote:
> 
> I think in C++ we don't need to say "enum" here.

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:212: enum FastOpenStatus fast_open_status_;
On 2013/09/13 22:22:44, wtc wrote:
> 
> I think in C++ we don't need to say "enum" here.

Done.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:214: // A connect operation is pending. In this
case, |write_callback_| needs to be
On 2013/09/13 22:22:44, wtc wrote:
> 
> In tcp_socket_win.h we use the read_callback_ for connect. Would be nice to
> be consistent. (I think it is better to use the write callback.)

Here we have to use write_callback_ because connect is notified by
OnFileCanWriteWithoutBlocking().

So I think we will need to change tcp_socket_win to be consistent.
Do you mind if that happens in a separate CL?

[commit-bot: I haz the power, 2013-09-18 04:46:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/23454010/33001

[commit-bot: I haz the power, 2013-09-18 06:05:50]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-18 19:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/23454010/73001

[commit-bot: I haz the power, 2013-09-18 21:50:28]

Change committed as 223945

[wtc, 2013-09-18 23:26:16]

Patch set 8 (what you committed) LGTM. Thanks!

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:158: Close();

On 2013/09/13 23:52:40, yzshen1 wrote:
> That will reset |tcp_fastopen_connected_| and cause the previous if-block to
> never run.

I see. So the original code worked because it never reset
tcp_fastopen_connected_ to false, right?

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:269: // completed and failed. Connecting the
same |socket_| again after a

On 2013/09/13 23:52:40, yzshen1 wrote:
>
> I thought this sentence also applies here? 
> Maybe I have misunderstood something?

Your comment is fine. Thanks.

(Since this won't work on POSIX, I thought the comment wasn't
necessary.)

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:417: DCHECK(address);

On 2013/09/13 23:52:40, yzshen1 wrote:
>
> Removing this part matches the behavior of tcp_socket_win.cc as well. What do
> you think?

Yes, this is fine.

If socket_ is kInvalidSocket (-1), I think
getsockname(socket_, ...) will fail with EBADF, which we
map to ERR_INVALID_HANDLE. That's a reasonable error code.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:586: return ERR_FAILED;

On 2013/09/13 23:52:40, yzshen1 wrote:
> I changed to ERR_ADDRESS_INVALID. Does it sound okay for accept to return
> ERR_ADDRESS_INVALID?

Yes. Since this failure is unlikely (note the NOTREACHED),
it doesn't matter in practice what we return as long as it's not
ERR_FAILED.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:690: if (read_callback_.is_null())

On 2013/09/13 23:52:40, yzshen1 wrote:
>
> Do you suggest me to remove it or we just stay at the safe side for now?

No change needed. I was just curious if you understand why.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:192: Watcher write_watcher_;

On 2013/09/13 23:52:40, yzshen1 wrote:
> I slightly prefer to have one Watcher object for each FileDescriptorWatcher.
> That is also what the original code did. What do you think?

We should use Watcher in the intended way. It is fine to
match the original code in the first iteration. If one
Watcher object can handle both Read and Write, we should
give that a try. (I am not familiar with Watcher, so I
don't know if this is a good idea.)

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:214: // A connect operation is pending. In this
case, |write_callback_| needs to be

On 2013/09/13 23:52:40, yzshen1 wrote:
>
> So I think we will need to change tcp_socket_win to be consistent.
> Do you mind if that happens in a separate CL?

No problem. Please feel free to do code improvements in a
separate CL.

[yzshen1, 2013-09-18 23:40:34]

Thank you Wan-Teh!

Btw, I realized I didn't reply to one of your question earlier:
> Did someone suggest the use of Closure in the Watcher class?
> That is one big change to the original code. This change is
> fine by me. I am just curious where it comes from.

I did this change because it looks nicer this way. Previously TCPSocketLibevent
inherited MessageLoopForIO::Watcher itself for Accept(). And migrating the
client socket operations would have added two more subclasses of
MessageLoopForIO::Watcher for Read() and Write(). It seems better to have a
single class to handle all the three cases.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:158: Close();
On 2013/09/18 23:26:17, wtc wrote:
> 
> On 2013/09/13 23:52:40, yzshen1 wrote:
> > That will reset |tcp_fastopen_connected_| and cause the previous if-block to
> > never run.
> 
> I see. So the original code worked because it never reset
> tcp_fastopen_connected_ to false, right?

Right. It is never set to false again.

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/23454010/diff/23001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.h:192: Watcher write_watcher_;
I see. I will look into the code and see whether it is okay to merge the two.

On 2013/09/18 23:26:17, wtc wrote:
> 
> On 2013/09/13 23:52:40, yzshen1 wrote:
> > I slightly prefer to have one Watcher object for each FileDescriptorWatcher.
> > That is also what the original code did. What do you think?
> 
> We should use Watcher in the intended way. It is fine to
> match the original code in the first iteration. If one
> Watcher object can handle both Read and Write, we should
> give that a try. (I am not familiar with Watcher, so I
> don't know if this is a good idea.)