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

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

R=sgjesse@google.com, wibling@google.com

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

[kustermann, 2015-01-28 16:40:15]

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

[kustermann, 2015-01-28 16:40:22]

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

[Søren Gjesse, 2015-01-29 09:05:51]

LGTM

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:238: sd->Close();
Move sd->Close() down below the removal from the map (like above).

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:61: // This socket creation is the exact same as the
one which created the
add "originally" after "which"

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:66: // We set as a side-effect the port on the dart
socket_object.
port -> file descriptor

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:89: ASSERT(allocated_port >= 0);
'>=' -> '>'

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:104: bool ListeningSocketRegistry::CloseSafe(int
socketfd) {
Maybe assert that the mutex is actually locked. As far as I remember our mutexes
are not recursive, so Mutex::TryLock() should fail.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:115: OSSocket *last = NULL;
Rename last to prev?

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:155: bool
ListeningSocketRegistry::addressesAreEqual(const RawAddr& a,
Move this as a static on SocketAddress where we have other RawAddr helpers.

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:8: #include <map>
As some point we where avoiding using STL, which is the reason for
platform/hashmap.h. However it looks like other parts of the VM is using STL
(with one comment "Remove STL dependencies"). Lets keep it like this.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:325: int socketfd;
To support Windows where this is a pointer this should be intptr_t.

https://codereview.chromium.org/879353003/diff/1/dart/sdk/lib/io/socket.dart
File dart/sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/879353003/diff/1/dart/sdk/lib/io/socket.dart#...
dart/sdk/lib/io/socket.dart:269: bool get shared;
Do we need to expose this as a public getter?

https://codereview.chromium.org/879353003/diff/1/dart/sdk/lib/io/socket.dart#...
dart/sdk/lib/io/socket.dart:367: bool get shared;
Ditto.

[wibling, 2015-01-29 09:42:01]

LGTM

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:238: sd->Close();
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> Move sd->Close() down below the removal from the map (like above).

Since it was closed before removal before shouldn't it be the above sd-Close()
that is moved before the map removal? Or was that a bug in the previous code?

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:352: // Returns `true` if the last reference has been
droped and the underlying
NIT: droped -> dropped

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:362: // NOTE: This is a temporary measure until
ServerSocketReferenc'es are
NIT: ServerSocketReferenc'es -> ServerSocketReference's

[Søren Gjesse, 2015-01-29 10:12:55]

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:238: sd->Close();
On 2015/01/29 09:42:01, wibling wrote:
> On 2015/01/29 09:05:51, Søren Gjesse wrote:
> > Move sd->Close() down below the removal from the map (like above).
> 
> Since it was closed before removal before shouldn't it be the above sd-Close()
> that is moved before the map removal? Or was that a bug in the previous code?

As far as I can see this could be a potential bug in the original code. You
close the fd, and then you might create a new one getting the same fd before it
is removed from the map. However as the eventhandler is single threaded it might
not be a problem...

[kustermann, 2015-01-29 11:11:29]

Patchset #2 (id:20001) has been deleted

[kustermann, 2015-01-29 11:14:05]

Addressed comments.

As discussed offline, I'll add more parts to this CL, since they need to land in
one go.

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:238: sd->Close();
On 2015/01/29 10:12:55, Søren Gjesse wrote:
> On 2015/01/29 09:42:01, wibling wrote:
> > On 2015/01/29 09:05:51, Søren Gjesse wrote:
> > > Move sd->Close() down below the removal from the map (like above).
> > 
> > Since it was closed before removal before shouldn't it be the above
sd-Close()
> > that is moved before the map removal? Or was that a bug in the previous
code?
> 
> As far as I can see this could be a potential bug in the original code. You
> close the fd, and then you might create a new one getting the same fd before
it
> is removed from the map. However as the eventhandler is single threaded it
might
> not be a problem...


Yes, because only one thread owns/touches this, it's not an issue. But it's much
cleaner to move it after the removal of the hashmap.

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:61: // This socket creation is the exact same as the
one which created the
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> add "originally" after "which"

Done.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:66: // We set as a side-effect the port on the dart
socket_object.
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> port -> file descriptor

Done.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:89: ASSERT(allocated_port >= 0);
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> '>=' -> '>'

Done.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:104: bool ListeningSocketRegistry::CloseSafe(int
socketfd) {
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> Maybe assert that the mutex is actually locked. As far as I remember our
mutexes
> are not recursive, so Mutex::TryLock() should fail.

Done.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.cc#n...
dart/runtime/bin/socket.cc:115: OSSocket *last = NULL;
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> Rename last to prev?

Done.

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

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:8: #include <map>
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> As some point we where avoiding using STL, which is the reason for
> platform/hashmap.h. However it looks like other parts of the VM is using STL
> (with one comment "Remove STL dependencies"). Lets keep it like this.

Yes, I thought there might be a 50/50 chance of be allowed to use it (I saw the
other uses in the VM as well).

It's not performance critical code, so the only potential downside is to
increase dart binary size I guess.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:325: int socketfd;
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> To support Windows where this is a pointer this should be intptr_t.

Done.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:352: // Returns `true` if the last reference has been
droped and the underlying
On 2015/01/29 09:42:01, wibling wrote:
> NIT: droped -> dropped

Done.

https://codereview.chromium.org/879353003/diff/1/dart/runtime/bin/socket.h#ne...
dart/runtime/bin/socket.h:362: // NOTE: This is a temporary measure until
ServerSocketReferenc'es are
On 2015/01/29 09:42:01, wibling wrote:
> NIT: ServerSocketReferenc'es -> ServerSocketReference's

Done.

https://codereview.chromium.org/879353003/diff/1/dart/sdk/lib/io/socket.dart
File dart/sdk/lib/io/socket.dart (right):

https://codereview.chromium.org/879353003/diff/1/dart/sdk/lib/io/socket.dart#...
dart/sdk/lib/io/socket.dart:269: bool get shared;
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> Do we need to expose this as a public getter?

No. I've removed it now. We can add it later on again if there is a need for it.

https://codereview.chromium.org/879353003/diff/1/dart/sdk/lib/io/socket.dart#...
dart/sdk/lib/io/socket.dart:367: bool get shared;
On 2015/01/29 09:05:51, Søren Gjesse wrote:
> Ditto.

Done.

[kustermann, 2015-01-29 11:21:19]

Patchset #3 (id:60001) has been deleted

[kustermann, 2015-01-29 11:22:33]

https://codereview.chromium.org/879353003/diff/80001/dart/runtime/bin/eventha...
File dart/runtime/bin/eventhandler_android.cc (left):

https://codereview.chromium.org/879353003/diff/80001/dart/runtime/bin/eventha...
dart/runtime/bin/eventhandler_android.cc:303:
reinterpret_cast<EventHandlerImplementation*>(args);

This was a pretty risky cast. It just happens by accident that

&EventHandler == &EventHandler.delegate_ 

(since delegate_ is the first instance member in the EventHandler class)

[kustermann, 2015-01-30 12:41:19]

Patchset #4 (id:100001) has been deleted

[kustermann, 2015-02-02 08:35:35]

Patchset #5 (id:140001) has been deleted

[kustermann, 2015-02-02 08:35:40]

Patchset #4 (id:120001) has been deleted

[kustermann, 2015-02-02 08:47:34]

Preliminary implementation for other platforms, pulls out the common
functionality (i.e. mask/ports) into super classes.

Since we're approaching 1.9 cut, you can take a look now, but I still have to
verify it on all platforms (and fix an issue on windows).

[Søren Gjesse, 2015-02-02 10:56:14]

Some comments (the Android comments apply to Linux and Mac OS as well).

The generic handling of one/multiple ports using HasNextPort/NextPort seems a
bit strange when handling with just one port. At least add some documentation in
the eventhandler.h file.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler.h:232: ASSERT(port_ == 0 || port == port_);
This assert looks strange. Are we adding the same port more than once?

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler.h:238: ASSERT(port_ == 0 || port_ == port);
Again, are we removing a port which is not added?

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:58: DescriptorInfoAndroid* si) {
si -> di

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:66: static void
AddToEpollInstance(intptr_t epoll_fd_, SocketData* si) {
SocketData -> DescriptorInfo/DescriptorInfoAndroid

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:82:
DartUtils::PostInt32(si->NextPort(), 1 << kCloseEvent);
How will this work for listening sockets with multiple ports? It should not
happen unless we don't keep correct track of closed listening sockets. Of course
there might be other errors.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:217: if (no_more_listeners) {
Isn't this always true for a non-listening socket? Then assert it instead.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_android.h (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.h:25: class DescriptorInfoAndroid : public
DescriptorInfo {
How about just calling the class DescriptorInfo here (no Android postfix) and
calling the base class DescriptorInfoDase?

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.h:40: class DescriptorInfoSingleLinux
DescriptorInfoSingleLinux -> DescriptorInfoSingle (see above).

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.h:48: class DescriptorInfoMultipleLinux
Ditto.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.cc:520: if (handle->IsClosed()) {
Indentation.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.cc:1006: // TODO(kustermann): on linux we do
On Windows we always have 5 outstanding accept requests. These are either
pending or completed and sitting in the linked list of accepted connections
ready to pass on to Dart.

[kustermann, 2015-02-03 10:45:35]

Update

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler.h:232: ASSERT(port_ == 0 || port == port_);
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> This assert looks strange. Are we adding the same port more than once?

I think so, I've observed this on windows I think. Previously it was just not
checked. I'll add a TODO to verify this.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler.h:238: ASSERT(port_ == 0 || port_ == port);
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> Again, are we removing a port which is not added?

ditto. I'll add a TODO.

If you want, I can also verify this first, before committing the CL.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:58: DescriptorInfoAndroid* si) {
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> si -> di

Originally I called the class SocketInfo, but it's used on linux for everything,
so it's DescriptorInfo now :-/

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:66: static void
AddToEpollInstance(intptr_t epoll_fd_, SocketData* si) {
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> SocketData -> DescriptorInfo/DescriptorInfoAndroid

Done.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:82:
DartUtils::PostInt32(si->NextPort(), 1 << kCloseEvent);
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> How will this work for listening sockets with multiple ports? It should not
> happen unless we don't keep correct track of closed listening sockets. Of
course
> there might be other errors.

I don't know what happens if we send a close event to the dart side. Whether the
dart side handles it correctly & sends a close command back (because if it
doesn't, we'll not close the underlying OS socket).

I've added a TODO for now.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.cc:217: if (no_more_listeners) {
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> Isn't this always true for a non-listening socket? Then assert it instead.

Done.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_android.h (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.h:25: class DescriptorInfoAndroid : public
DescriptorInfo {
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> How about just calling the class DescriptorInfo here (no Android postfix) and
> calling the base class DescriptorInfoDase?

Done. Also renamed the other two to Mixins.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.h:40: class DescriptorInfoSingleLinux
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> DescriptorInfoSingleLinux -> DescriptorInfoSingle (see above).

Done.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_android.h:48: class DescriptorInfoMultipleLinux
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> Ditto.

Done.

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/879353003/diff/160001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.cc:520: if (handle->IsClosed()) {
On 2015/02/02 10:56:14, Søren Gjesse wrote:
> Indentation.

Done.

[kustermann, 2015-02-04 11:50:26]

PTAL - running all tests in all configurations on windows now, but standalone/io
seems to work.

[Søren Gjesse, 2015-02-04 12:19:58]

LGTM

https://codereview.chromium.org/879353003/diff/180001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/879353003/diff/180001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_macos.cc:253: // Add/Remove from epoll set
depending on previous and current state.
epoll set -> kqueue

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler.h:389: }
Maybe have an #ifdef DEBUG section checking that all active_readers are ready.

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.cc:1121: if (listen_socket->HasNextPort()) {
You could put the condition 'listen_socket->HasNextPort()' into the for loop to
avoid the break.

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.h:402: int pending_accept_count_;
It would probably be good to add a comment on the difference between the
pending_accept_count_ and accepted_count_.

[kustermann, 2015-02-04 13:38:24]

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

[kustermann, 2015-02-04 13:39:01]

Patchset #7 (id:220001) has been deleted

[kustermann, 2015-02-04 13:39:43]

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler.h:389: }
On 2015/02/04 12:19:57, Søren Gjesse wrote:
> Maybe have an #ifdef DEBUG section checking that all active_readers are ready.

Done.

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.cc:1121: if (listen_socket->HasNextPort()) {
On 2015/02/04 12:19:57, Søren Gjesse wrote:
> You could put the condition 'listen_socket->HasNextPort()' into the for loop
to
> avoid the break.

Done.

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
File dart/runtime/bin/eventhandler_win.h (right):

https://codereview.chromium.org/879353003/diff/200001/dart/runtime/bin/eventh...
dart/runtime/bin/eventhandler_win.h:402: int pending_accept_count_;
On 2015/02/04 12:19:57, Søren Gjesse wrote:
> It would probably be good to add a comment on the difference between the
> pending_accept_count_ and accepted_count_.

Done.

[kustermann, 2015-02-04 13:42:02]

Committed patchset #7 (id:240001) manually as 43467 (presubmit successful).