Reland "Introduce optional 'bool shared' parameter to ServerSocket.bind() ..."

Reland "Introduce optional 'bool shared' parameter to ServerSocket.bind() ..."

R=sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43678

[kustermann, 2015-02-10 16:35:51]

kustermann@google.com changed reviewers:
+ sgjesse@google.com

[kustermann, 2015-02-10 16:35:51]

https://codereview.chromium.org/913753002/diff/1/dart/runtime/bin/eventhandle...
File dart/runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/913753002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_win.cc:1030: listen_socket->Close();
Note that the `Handle::ScopedLock lock()` from above doesn't conflict with the
lock aquired in Close() because the implementation of ScopedLock on windows
seems to allow a thread to take the same lock multiple times on the same thread
:-/

https://codereview.chromium.org/913753002/diff/1/dart/tests/standalone/io/soc...
File dart/tests/standalone/io/socket_bind_test.dart (right):

https://codereview.chromium.org/913753002/diff/1/dart/tests/standalone/io/soc...
dart/tests/standalone/io/socket_bind_test.dart:74: addr2, socket.port, v6Only:
addr2V6Only, shared: false).then((socket2) {
There's a very small race here -- we assume that the port used by the previous
socket we created is still available for another address (which might not be the
case).

I don't know a good way of making this bullet proof.

[Søren Gjesse, 2015-02-11 08:51:05]

LGTM!

https://codereview.chromium.org/913753002/diff/1/dart/runtime/bin/eventhandle...
File dart/runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/913753002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_win.cc:1030: listen_socket->Close();
On 2015/02/10 16:35:51, kustermann wrote:
> Note that the `Handle::ScopedLock lock()` from above doesn't conflict with the
> lock aquired in Close() because the implementation of ScopedLock on windows
> seems to allow a thread to take the same lock multiple times on the same
thread
> :-/

It uses a Windows critical section object which is always "recursive".

https://codereview.chromium.org/913753002/diff/1/dart/tests/standalone/io/soc...
File dart/tests/standalone/io/socket_bind_test.dart (right):

https://codereview.chromium.org/913753002/diff/1/dart/tests/standalone/io/soc...
dart/tests/standalone/io/socket_bind_test.dart:74: addr2, socket.port, v6Only:
addr2V6Only, shared: false).then((socket2) {
On 2015/02/10 16:35:51, kustermann wrote:
> There's a very small race here -- we assume that the port used by the previous
> socket we created is still available for another address (which might not be
the
> case).
> 
> I don't know a good way of making this bullet proof.

That is probably not possible to be sure unless nothing else is running at the
same time. Retrying 10 times would being the probability further down.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/eventha...
File dart/runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/eventha...
dart/runtime/bin/eventhandler_macos.cc:247: MutexLocker
locker(registry->mutex());
nit: Formatting (number of empty lines) of this section which is otherwise
identical on the 4 platforms is different.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/main.cc
File dart/runtime/bin/main.cc (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/main.cc...
dart/runtime/bin/main.cc:952: ListeningSocketRegistry::Initialize();
Can't this go inside EventHandler::Start()?

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket.cc
File dart/runtime/bin/socket.cc (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.cc:27: bool ListeningSocketRegistry::Initialize() {
Just void return type?

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.cc:86: // the file descriptor.
'return the file descriptor' -> 'reuse the file descriptor'

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.cc:149: if (prev == NULL && current->next == NULL) {
Please add a few comments here

1. Removed last element from linked list

2. Removed first element from linked list

3. Removed element from linked list which was not the first.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket.h
File dart/runtime/bin/socket.h (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.h:409: typedef std::map<intptr_t, OSSocket*>::iterator
SocketsIterator;
Please add

DISALLOW_COPY_AND_ASSIGN(ListeningSocketRegistry);

https://codereview.chromium.org/913753002/diff/20001/dart/tests/standalone/io...
File dart/tests/standalone/io/socket_bind_test.dart (right):

https://codereview.chromium.org/913753002/diff/20001/dart/tests/standalone/io...
dart/tests/standalone/io/socket_bind_test.dart:15: 'localhost', 0, v6Only:
v6Only, shared: true).then((socket) {
Consider using '127.0.0.1', as 'localhost' normally resolves to 127.0.0.1 on
Linux and ::1 on Windows. Maybe test on both 127.0.0.1 and ::1.

[kustermann, 2015-02-11 09:55:41]

New patchsets have been uploaded after l-g-t-m from sgjesse@google.com

[kustermann, 2015-02-11 09:56:09]

Thanks - addressed comments and tested (on linux).

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/eventha...
File dart/runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/eventha...
dart/runtime/bin/eventhandler_macos.cc:247: MutexLocker
locker(registry->mutex());
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> nit: Formatting (number of empty lines) of this section which is otherwise
> identical on the 4 platforms is different.

Done.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/main.cc
File dart/runtime/bin/main.cc (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/main.cc...
dart/runtime/bin/main.cc:952: ListeningSocketRegistry::Initialize();
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> Can't this go inside EventHandler::Start()?

Done.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket.cc
File dart/runtime/bin/socket.cc (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.cc:27: bool ListeningSocketRegistry::Initialize() {
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> Just void return type?

Done.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.cc:86: // the file descriptor.
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> 'return the file descriptor' -> 'reuse the file descriptor'

Done.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.cc:149: if (prev == NULL && current->next == NULL) {
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> Please add a few comments here
> 
> 1. Removed last element from linked list
> 
> 2. Removed first element from linked list
> 
> 3. Removed element from linked list which was not the first.

Done.

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket.h
File dart/runtime/bin/socket.h (right):

https://codereview.chromium.org/913753002/diff/20001/dart/runtime/bin/socket....
dart/runtime/bin/socket.h:409: typedef std::map<intptr_t, OSSocket*>::iterator
SocketsIterator;
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> Please add
> 
> DISALLOW_COPY_AND_ASSIGN(ListeningSocketRegistry);

Done.

https://codereview.chromium.org/913753002/diff/20001/dart/tests/standalone/io...
File dart/tests/standalone/io/socket_bind_test.dart (right):

https://codereview.chromium.org/913753002/diff/20001/dart/tests/standalone/io...
dart/tests/standalone/io/socket_bind_test.dart:15: 'localhost', 0, v6Only:
v6Only, shared: true).then((socket) {
On 2015/02/11 08:51:05, Søren Gjesse wrote:
> Consider using '127.0.0.1', as 'localhost' normally resolves to 127.0.0.1 on
> Linux and ::1 on Windows. Maybe test on both 127.0.0.1 and ::1.

Done.

[kustermann, 2015-02-11 10:01:44]

Committed patchset #3 (id:40001) manually as 43678 (presubmit successful).