Allow server sockets to rebind to same port if there is nothing actively listening on that port.

Allow server sockets to rebind to same port if there is nothing actively listening on that port.


BUG=


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

[justinlin, 2012-09-10 21:01:20]

Hi Sergey, please take a look. This is a change I need to implement the chrome
apps socket.bind api.

[Sergey Ulanov, 2012-09-11 23:57:45]

we have AllowAddressReuse() method for UDP sockets. Maybe add the same method
for TCP server sockets for the case when we don't need this flag.

[justinlin, 2012-09-12 04:54:44]

On 2012/09/11 23:57:45, sergeyu wrote:
> we have AllowAddressReuse() method for UDP sockets. Maybe add the same method
> for TCP server sockets for the case when we don't need this flag.

Done, please take another look. Thanks!

[Sergey Ulanov, 2012-09-12 19:38:24]

looks mostly good, but I don't think you need bitmap for socket flags - just use
bool variable. Also new members need initializers.

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

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:47: enum SocketOptions {
types must be defined before methods.

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:59: int socket_options_;
Do we really need these options to be bit flags? it would be cleaner to have
boolean flag reuse_address.

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:59: int socket_options_;
You need to initialize this variable in the constructor.

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_win.h (right):

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.h:51: }
I think you are missing semicolon here

[justinlin, 2012-09-12 21:11:00]

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

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:47: enum SocketOptions {
On 2012/09/12 19:38:24, sergeyu wrote:
> types must be defined before methods.

Ack.

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:59: int socket_options_;
On 2012/09/12 19:38:24, sergeyu wrote:
> Do we really need these options to be bit flags? it would be cleaner to have
> boolean flag reuse_address.

Done.

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:59: int socket_options_;
On 2012/09/12 19:38:24, sergeyu wrote:
> You need to initialize this variable in the constructor.

Done.

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_win.h (right):

http://codereview.chromium.org/10907154/diff/2002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.h:51: }
On 2012/09/12 19:38:24, sergeyu wrote:
> I think you are missing semicolon here
Ack

[Sergey Ulanov, 2012-09-12 21:52:35]

LGTM provided my style nits are addressed. You also need an OWNERS review.

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:199: void
TCPServerSocketLibevent::AllowAddressReuse() {
nit: It's best to put method implementations in the same order as in the header.
Put this one after Accept()?

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:41: void AllowAddressReuse();
nit: put this above MessageLoopForIO::Watcher

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:46: int SetSocketOptions();
nit: I would place it before AcceptInternal() to reflect the order in which this
methods are normally called.

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:55: bool reuse_addr_;
nit: reuse_address_

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.cc:24: reuse_addr(false),
typo: reuse_addr_
And rename it to reuse_address_.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.h:43: void AllowAddressReuse();
Move this above base::ObjectWatcher::Delegate

[justinlin, 2012-09-13 07:12:56]

Done, wtc@, willchan@, please take a look for OWNERS approval. Thanks.

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:199: void
TCPServerSocketLibevent::AllowAddressReuse() {
On 2012/09/12 21:52:35, sergeyu wrote:
> nit: It's best to put method implementations in the same order as in the
header.
> Put this one after Accept()?

Done.

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:41: void AllowAddressReuse();
On 2012/09/12 21:52:35, sergeyu wrote:
> nit: put this above MessageLoopForIO::Watcher

Done.

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:46: int SetSocketOptions();
On 2012/09/12 21:52:35, sergeyu wrote:
> nit: I would place it before AcceptInternal() to reflect the order in which
this
> methods are normally called.

Done.

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:55: bool reuse_addr_;
On 2012/09/12 21:52:35, sergeyu wrote:
> nit: reuse_address_

Done.

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.h:43: void AllowAddressReuse();
On 2012/09/12 21:52:35, sergeyu wrote:
> Move this above base::ObjectWatcher::Delegate

Done.

[wtc, 2012-09-13 18:31:55]

Review comments on patch set 5:

I suggest some changes. Thanks!

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:46: int SetSocketOptions();

On 2012/09/12 21:52:35, sergeyu wrote:
> nit: I would place it before AcceptInternal() to reflect the order in which
this
> methods are normally called.

This seems like a good idea. To be consistent, should we
also declare AllowAddressReuse() before Listen()?

NOTE: I noticed the mismatch of declaration and definition
orders between .h and .cc. But I think the original code
already has a mismatch, and it seems that you have done
your best to match the order.

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

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:68: if (result < 0) {

result < 0 => result != OK

Note: although SetSocketOptions() and bind()/listen() all
return int, SetSocketOptions() returns a Chrome network
error code, whereas bind()/listen() return 0/-1 (the Unix
system call status values).

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:147: return MapSystemError(errno);

Why didn't you copy the following code from tcp_client_socket_libevent.cc?

#if defined(SO_REUSEPORT)
    rv = setsockopt(socket_, SOL_SOCKET, SO_REUSEPORT, &true_value,
                    sizeof(true_value));
    if (rv < 0)
      return MapSystemError(errno);
#endif

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

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:34: // Sets corresponding flags to allow
the socket to share the local address to

Please make the following changes to tcp_server_socket_win.h
as well.

The AllowAddressReuse() method should be added to the
ServerSocket interface.

Remove "Sets corresponding flags to" from this comment.
This comment should start with "Allows the socket to share ..."

Change "Bind()" to "Listen()".

[wtc, 2012-09-13 18:32:56]

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_win.cc (right):

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.cc:61: if (result < 0) {

Nit: this should also be changed to result != OK.

[Sergey Ulanov, 2012-09-13 22:08:44]

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

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:147: return MapSystemError(errno);
On 2012/09/13 18:31:55, wtc wrote:
> 
> Why didn't you copy the following code from tcp_client_socket_libevent.cc?
> 
> #if defined(SO_REUSEPORT)
>     rv = setsockopt(socket_, SOL_SOCKET, SO_REUSEPORT, &true_value,
>                     sizeof(true_value));
>     if (rv < 0)
>       return MapSystemError(errno);
> #endif

I actually don't think we really need this. Yuri added it specifically for
broadcast sockets on mac (though I don't know all details of why it's required
there). See my comments on that issue here:
http://codereview.chromium.org/10739002/ . There is also CL in flight to fix it:
http://codereview.chromium.org/10917235/

[wtc, 2012-09-14 02:36:12]

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

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:147: return MapSystemError(errno);

On 2012/09/13 22:08:45, sergeyu wrote:
>
> I actually don't think we really need this.

Thank you. Then we should not copy the SO_REUSEPORT code to
TCPServerSocket.

[justinlin, 2012-09-14 03:54:40]

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

http://codereview.chromium.org/10907154/diff/8001/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:46: int SetSocketOptions();
On 2012/09/13 18:31:55, wtc wrote:
> 
> On 2012/09/12 21:52:35, sergeyu wrote:
> > nit: I would place it before AcceptInternal() to reflect the order in which
> this
> > methods are normally called.
> 
> This seems like a good idea. To be consistent, should we
> also declare AllowAddressReuse() before Listen()?
> 
> NOTE: I noticed the mismatch of declaration and definition
> orders between .h and .cc. But I think the original code
> already has a mismatch, and it seems that you have done
> your best to match the order.
> 

Done.

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

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:68: if (result < 0) {
On 2012/09/13 18:31:55, wtc wrote:
> 
> result < 0 => result != OK
> 
> Note: although SetSocketOptions() and bind()/listen() all
> return int, SetSocketOptions() returns a Chrome network
> error code, whereas bind()/listen() return 0/-1 (the Unix
> system call status values).

Done. Nice catch.

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

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.h:34: // Sets corresponding flags to allow
the socket to share the local address to
On 2012/09/13 18:31:55, wtc wrote:
> 
> Please make the following changes to tcp_server_socket_win.h
> as well.
> 
> The AllowAddressReuse() method should be added to the
> ServerSocket interface.
> 
> Remove "Sets corresponding flags to" from this comment.
> This comment should start with "Allows the socket to share ..."
> 
> Change "Bind()" to "Listen()".

Done. Also moved the comment to interface.

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_win.cc (right):

http://codereview.chromium.org/10907154/diff/5002/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.cc:61: if (result < 0) {
On 2012/09/13 18:32:56, wtc wrote:
> 
> Nit: this should also be changed to result != OK.

Done.

[wtc, 2012-09-14 19:20:17]

Patch set 6 LGTM. You can commit this after making the
suggested changes. No need to wait for another review.

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

http://codereview.chromium.org/10907154/diff/6004/net/socket/server_socket.h#...
net/socket/server_socket.h:24: virtual void AllowAddressReuse();

This should be a pure virtual method, with = 0.

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

http://codereview.chromium.org/10907154/diff/6004/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:75: if (result != OK) {

Remove the curly braces {}.

Also in tcp_server_socket_win.cc.

http://codereview.chromium.org/10907154/diff/6004/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_win.h (right):

http://codereview.chromium.org/10907154/diff/6004/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.h:39: virtual void OnObjectSignaled(HANDLE
object);

Could you add OVERRIDE to all these overridden methods?
Thank you for your help.

[justinlin, 2012-09-15 19:03:25]

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

http://codereview.chromium.org/10907154/diff/6004/net/socket/server_socket.h#...
net/socket/server_socket.h:24: virtual void AllowAddressReuse();
On 2012/09/14 19:20:17, wtc wrote:
> 
> This should be a pure virtual method, with = 0.

Done.

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

http://codereview.chromium.org/10907154/diff/6004/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_libevent.cc:75: if (result != OK) {
On 2012/09/14 19:20:17, wtc wrote:
> 
> Remove the curly braces {}.
> 
> Also in tcp_server_socket_win.cc.

Done.

http://codereview.chromium.org/10907154/diff/6004/net/socket/tcp_server_socke...
File net/socket/tcp_server_socket_win.h (right):

http://codereview.chromium.org/10907154/diff/6004/net/socket/tcp_server_socke...
net/socket/tcp_server_socket_win.h:39: virtual void OnObjectSignaled(HANDLE
object);
On 2012/09/14 19:20:17, wtc wrote:
> 
> Could you add OVERRIDE to all these overridden methods?
> Thank you for your help.

Done.

[commit-bot: I haz the power, 2012-09-15 19:04:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/10907154/8007

[commit-bot: I haz the power, 2012-09-15 19:29:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-17 04:36:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/10907154/14

[commit-bot: I haz the power, 2012-09-17 06:59:41]

Change committed as 157087

[wtc, 2012-09-17 22:41:09]

Patch set 8 LGTM.