Added TCPServerSocket.

Added TCPServerSocket.

BUG=None
TEST=Unittests

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

[Sergey Ulanov, 2011-04-12 00:03:44]

TCP server sockets are needed for P2P connections.

[willchan no longer on Chromium, 2011-04-12 16:45:00]

Minor nits while I think this over. I'm wondering if the introduction of server
sockets means we need to rethink this whole client socket and server socket
distinction. It's weird that a server socket produces a client socket. Maybe we
should have a ConnectedSocket interface. I need to think it over and perhaps
discuss with mbelshe.

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.cc (right):

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:46: close(socket_);
HANDLE_EINTR

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.h (right):

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.h:18: private MessageLoopForIO::Watcher {
Don't use private inheritance please
(http://www.parashift.com/c++-faq-lite/private-inheritance.html#faq-24.2 and
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...).

[Sergey Ulanov, 2011-04-12 19:20:46]

I agree, that it doesn't feel right that the server socket returns client
sockets. Your suggestions on how this design can be improved are welcome.

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.cc (right):

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:46: close(socket_);
On 2011/04/12 16:45:00, willchan wrote:
> HANDLE_EINTR

Done.

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.h (right):

http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.h:18: private MessageLoopForIO::Watcher {
On 2011/04/12 16:45:00, willchan wrote:
> Don't use private inheritance please
> (http://www.parashift.com/c++-faq-lite/private-inheritance.html#faq-24.2 and
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...).

Done.

[willchan no longer on Chromium, 2011-04-13 15:14:03]

I sorta think a good first step if to ditch the ClientSocket concept and replace
it with ConnectedSocket. This is a big/annoying rename. How about I not block
you on that change, I talk it over with mbelshe, and if we agree, then I'll ask
you to do the rename after submitting this change?

On 2011/04/12 19:20:46, sergeyu wrote:
> I agree, that it doesn't feel right that the server socket returns client
> sockets. Your suggestions on how this design can be improved are welcome.
> 
>
http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
> File net/socket/tcp_server_socket_libevent.cc (right):
> 
>
http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
> net/socket/tcp_server_socket_libevent.cc:46: close(socket_);
> On 2011/04/12 16:45:00, willchan wrote:
> > HANDLE_EINTR
> 
> Done.
> 
>
http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
> File net/socket/tcp_server_socket_libevent.h (right):
> 
>
http://codereview.chromium.org/6820057/diff/6012/net/socket/tcp_server_socket...
> net/socket/tcp_server_socket_libevent.h:18: private MessageLoopForIO::Watcher
{
> On 2011/04/12 16:45:00, willchan wrote:
> > Don't use private inheritance please
> > (http://www.parashift.com/c++-faq-lite/private-inheritance.html#faq-24.2 and
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...).
> 
> Done.

[willchan no longer on Chromium, 2011-04-13 16:08:51]

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.cc (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:41: net_log_source_(source) {
You should probably be creating new NetLog events and logging something on all
these API calls.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:148: *socket = new
TCPClientSocket(AddressList(), net_log_, net_log_source_);
You should probably be constructing a AddressList from |address| and passing
that in.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:164: if (result == 0) {
result == OK

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:169: c->Run(0);
Run(OK)

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.h (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.h:24: // Bind the socket and start
listening.
I guess it's implicit, but you might want to document that the way to stop
listening is to destroy the socket.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.h:32: int Accept(TCPClientSocket** socket,
IPEndPoint* address,
http://dev.chromium.org/developers/coding-style#Code_Formatting says:
For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many":

What do you think about using a scoped_ptr<TCPClientSocket>* instead to indicate
ownership of the result? And is this |address| parameter necessary? Can't we get
this by ClientSocket::GetPeerAddress()? Or do you think it'll be used often?

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_unittest.cc (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:9: #include
"net/socket/tcp_server_socket.h"
This include goes first.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:17: int kListenBacklog = 5;
const

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:18: }  // namespace
How about putting all of this test code in the anonymous namespace?

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:38: scoped_ptr<TCPServerSocket>
socket_;
Why are you using a scoped_ptr? Why not just directly embed the socket and
initialize it in the constructor's initializer list?

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:44: scoped_ptr<TCPClientSocket>
connecting_socket(
I don't see why you use a scoped_ptr here instead of just allocating the
TCPClientSocket on the stack. Same with everywhere else.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:57: ASSERT_EQ(0, result);
Use OK instead of 0.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:63: EXPECT_EQ(0,
connect_callback.WaitForResult());
Ditto, please fix all other instances.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_win.cc (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.cc:9: #include "base/eintr_wrapper.h"
This should not be needed.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.cc:137: *socket = new
TCPClientSocket(AddressList(), net_log_, net_log_source_);
You should fix AddressList() here too.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.cc:144: HANDLE_EINTR(closesocket(socket_));
HANDLE_EINTR isn't necessary, it's just for POSIX.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_win.h (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.h:35: int Accept(TCPClientSocket** socket,
IPEndPoint* address,
One argument per line.

[Sergey Ulanov, 2011-04-13 21:20:19]

Also added ServerSocket interface.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.cc (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:41: net_log_source_(source) {
On 2011/04/13 16:08:51, willchan wrote:
> You should probably be creating new NetLog events and logging something on all
> these API calls.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:148: *socket = new
TCPClientSocket(AddressList(), net_log_, net_log_source_);
On 2011/04/13 16:08:51, willchan wrote:
> You should probably be constructing a AddressList from |address| and passing
> that in.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:164: if (result == 0) {
On 2011/04/13 16:08:51, willchan wrote:
> result == OK
Actually this code was incorrect: Callback should be called for any result value
except ERR_IO_PENDING.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.cc:169: c->Run(0);
On 2011/04/13 16:08:51, willchan wrote:
> Run(OK)

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_libevent.h (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.h:24: // Bind the socket and start
listening.
On 2011/04/13 16:08:51, willchan wrote:
> I guess it's implicit, but you might want to document that the way to stop
> listening is to destroy the socket.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_libevent.h:32: int Accept(TCPClientSocket** socket,
IPEndPoint* address,
On 2011/04/13 16:08:51, willchan wrote:
> http://dev.chromium.org/developers/coding-style#Code_Formatting says:
> For function declarations and definitions, put each argument on a separate
line
> unless everything fits on one line. You should only break this rule when the
> arguments have some logical association like "int x, int y" or "char* str, int
> str_len" where putting a few arguments together makes semantic sense.  Do NOT
> collapse function arguments together just because you have "too many":
Done.


> What do you think about using a scoped_ptr<TCPClientSocket>* instead to
indicate
> ownership of the result?
Done

> And is this |address| parameter necessary? Can't we get
> this by ClientSocket::GetPeerAddress()? Or do you think it'll be used often?
In most cases caller would need the address, but I don't see the reason why
GetPeerAddress() cannot be used. I was initially thinking that having it here
would save us one syscall, but it's not the case because GetPeerAddress()
doesn't make any syscalls.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_unittest.cc (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:9: #include
"net/socket/tcp_server_socket.h"
On 2011/04/13 16:08:51, willchan wrote:
> This include goes first.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:17: int kListenBacklog = 5;
On 2011/04/13 16:08:51, willchan wrote:
> const

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:18: }  // namespace
On 2011/04/13 16:08:51, willchan wrote:
> How about putting all of this test code in the anonymous namespace?

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:38: scoped_ptr<TCPServerSocket>
socket_;
On 2011/04/13 16:08:51, willchan wrote:
> Why are you using a scoped_ptr? Why not just directly embed the socket and
> initialize it in the constructor's initializer list?

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:44: scoped_ptr<TCPClientSocket>
connecting_socket(
On 2011/04/13 16:08:51, willchan wrote:
> I don't see why you use a scoped_ptr here instead of just allocating the
> TCPClientSocket on the stack. Same with everywhere else.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:57: ASSERT_EQ(0, result);
On 2011/04/13 16:08:51, willchan wrote:
> Use OK instead of 0.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_unittest.cc:63: EXPECT_EQ(0,
connect_callback.WaitForResult());
On 2011/04/13 16:08:51, willchan wrote:
> Ditto, please fix all other instances.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_win.cc (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.cc:9: #include "base/eintr_wrapper.h"
On 2011/04/13 16:08:51, willchan wrote:
> This should not be needed.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.cc:137: *socket = new
TCPClientSocket(AddressList(), net_log_, net_log_source_);
On 2011/04/13 16:08:51, willchan wrote:
> You should fix AddressList() here too.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.cc:144: HANDLE_EINTR(closesocket(socket_));
On 2011/04/13 16:08:51, willchan wrote:
> HANDLE_EINTR isn't necessary, it's just for POSIX.

Done.

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
File net/socket/tcp_server_socket_win.h (right):

http://codereview.chromium.org/6820057/diff/8001/net/socket/tcp_server_socket...
net/socket/tcp_server_socket_win.h:35: int Accept(TCPClientSocket** socket,
IPEndPoint* address,
On 2011/04/13 16:08:51, willchan wrote:
> One argument per line.

Done.

[Sergey Ulanov, 2011-04-15 18:59:54]

ping

[willchan no longer on Chromium, 2011-04-18 18:07:17]

http://codereview.chromium.org/6820057/diff/14001/net/socket/server_socket.h
File net/socket/server_socket.h (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/server_socket.h#...
net/socket/server_socket.h:16: class ServerSocket {
Can you justify the need for this abstraction?

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_libevent.cc (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:113: int
TCPServerSocketLibevent::Accept(
You should probably add NetLog to this member function, and log the Accept()
call and return value (when it completes).

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_libevent.h (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:30: CompletionCallback* callback);
You should include net/base/completion_callback.h for this definition.

[Sergey Ulanov, 2011-04-18 19:40:28]

http://codereview.chromium.org/6820057/diff/14001/net/socket/server_socket.h
File net/socket/server_socket.h (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/server_socket.h#...
net/socket/server_socket.h:16: class ServerSocket {
On 2011/04/18 18:07:17, willchan wrote:
> Can you justify the need for this abstraction?
That's mainly to make it easier to unittest code that uses TCPServerSocket. I
could just make corresponding methods virtual in TCPSocketServer, but I think
having an interface is cleaner. I'll remove it if you disagree.

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_libevent.cc (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:113: int
TCPServerSocketLibevent::Accept(
On 2011/04/18 18:07:17, willchan wrote:
> You should probably add NetLog to this member function, and log the Accept()
> call and return value (when it completes).

Done.

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_libevent.h (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:30: CompletionCallback* callback);
On 2011/04/18 18:07:17, willchan wrote:
> You should include net/base/completion_callback.h for this definition.

Done.

[willchan no longer on Chromium, 2011-04-18 20:08:56]

LGTM, although I may ask you to rename the returned socket from ClientSocket to
something else, if Mike and I can reach a consensus.

http://codereview.chromium.org/6820057/diff/14001/net/socket/server_socket.h
File net/socket/server_socket.h (right):

http://codereview.chromium.org/6820057/diff/14001/net/socket/server_socket.h#...
net/socket/server_socket.h:16: class ServerSocket {
On 2011/04/18 19:40:28, sergeyu wrote:
> On 2011/04/18 18:07:17, willchan wrote:
> > Can you justify the need for this abstraction?
> That's mainly to make it easier to unittest code that uses TCPServerSocket. I
> could just make corresponding methods virtual in TCPSocketServer, but I think
> having an interface is cleaner. I'll remove it if you disagree.

I see. I had thought you were planning on using this polymorphically in
production code. Coding to interfaces is good and makes testing easier, so I'm
fine with this as is.

[Sergey Ulanov, 2011-04-18 20:30:49]

On 2011/04/18 20:08:56, willchan wrote:
> LGTM, although I may ask you to rename the returned socket from ClientSocket
to
> something else, if Mike and I can reach a consensus.
SGTM