Add support for cloning server-sockets.

Add support for cloning server-sockets.

BUG=
R=lrn@google.com, sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2014-04-24 12:46:47]

Initial impression.

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

https://codereview.chromium.org/250513002/diff/20001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:251: * Returns a [RawServerSocketHandle] that can be sent
to other isolates. To
This feels wrong.

It seems there is some underlying concept, the "real socket" that receives
connections. These are then forwarded to currently registered receivers for that
socket.
These receivers are what we currently call "RawServerSocket", but they are not,
in fact, *raw* server sockets. There is a layer in-between.

The RawServerSocketHandle is not a "Handle". It is the *real* socket.

Is this a workaround to avoid changing the name of RawServerSocket?
Could we do that? I.e.:
 RawServerSocketReceiver mySock = new RawServerSocketReceiver();
 RawServerSocket realSock = mySock.rawSocket;  // instead of clone()
 RawServerSocketReceiver mySock2 = realSock.createReceiver();

The "Handle" name is meaningless in any case, and should be improved. To what
depends on what it really means.

[Søren Gjesse, 2014-04-25 12:24:44]

Except for the naming this looks good. Let's think more about that.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
File runtime/bin/eventhandler_linux.h (right):

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:32: class LinkedList {
Maybe call this CircularLinkedList as it is circular.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:161: return static_cast<uint32_t>(port);
Use (port & 0xffffffff).

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:178: entry->value = reinterpret_cast<void*>(4);
Please add a constant for this.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:841: if (tokens == 2) {
Please add a constant for this.

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

https://codereview.chromium.org/250513002/diff/20001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:251: * Returns a [RawServerSocketHandle] that can be sent
to other isolates. To
On 2014/04/24 12:46:47, Lasse Reichstein Nielsen wrote:
> This feels wrong.
> 
> It seems there is some underlying concept, the "real socket" that receives
> connections. These are then forwarded to currently registered receivers for
that
> socket.
> These receivers are what we currently call "RawServerSocket", but they are
not,
> in fact, *raw* server sockets. There is a layer in-between.
> 
> The RawServerSocketHandle is not a "Handle". It is the *real* socket.
> 
> Is this a workaround to avoid changing the name of RawServerSocket?
> Could we do that? I.e.:
>  RawServerSocketReceiver mySock = new RawServerSocketReceiver();
>  RawServerSocket realSock = mySock.rawSocket;  // instead of clone()
>  RawServerSocketReceiver mySock2 = realSock.createReceiver();
> 
> The "Handle" name is meaningless in any case, and should be improved. To what
> depends on what it really means.

We already discussed that the naming is difficult. Clone dosen't realy clone,
but provides a "token" to "attach" to attach to a listening socket - given that
it was not already closed. Acquire doen not really "acquire" the socket.

Starting backwards the "acquire" method should maybe be named "attach" as it
uses a "token" to attach to an existing server socket. What to call the token
and the method to get it I currently have no realy good idea.

RawServerSocketToken
RawServerSocketLocater
RawServerSocketProxy (no)

[Anders Johnsen, 2014-05-01 11:34:49]

I wish I had a better idea for a name. Let's put our heads together tomorrow and
discuss offline.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
File runtime/bin/eventhandler_linux.h (right):

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:32: class LinkedList {
On 2014/04/25 12:24:44, Søren Gjesse wrote:
> Maybe call this CircularLinkedList as it is circular.

Done.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:161: return static_cast<uint32_t>(port);
On 2014/04/25 12:24:44, Søren Gjesse wrote:
> Use (port & 0xffffffff).

Done.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:178: entry->value = reinterpret_cast<void*>(4);
On 2014/04/25 12:24:44, Søren Gjesse wrote:
> Please add a constant for this.

Done.

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/250513002/diff/20001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:841: if (tokens == 2) {
On 2014/04/25 12:24:44, Søren Gjesse wrote:
> Please add a constant for this.

Done.

[Anders Johnsen, 2014-05-06 06:54:33]

PTAL

[Søren Gjesse, 2014-05-06 08:47:38]

lgtm

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1090: ReceivePort _port;
Call this variable something less generic, e.g. _referencePort.

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

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:257: * WARNING: This feature is highly *experimental* and
currently only works on
Move the WARNING up as the first line of the comment.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:264: /**
Add a WARNING here as well.

WARNING: This class is used with RawServerSocket.reference which is highly
*experimental* ...

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:345: * WARNING: This feature is highly *experimental* and
currently only works on
Move WARNING up.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:348: RawServerSocketReference get reference;
Return type ServerSockerReference

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:352: /**
Add WARNING

[Lasse Reichstein Nielsen, 2014-05-06 09:04:20]

lgtm

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
File runtime/bin/eventhandler_linux.h (right):

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:43: } else {
Add comment, either on method or here:
 Adds entry just before head.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:47: e->next_->prev_ = e;
Just:
  head_->prev = e;

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:70: void MoveNext() {
Could this be named something with "rotate"?

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:163: return static_cast<uint32_t>(port &
0xFFFFFFFF);
Why the '& 0xFFFFFFFF' ?
Casting an int64_t to uint32_t should just pick the low 32 bits anyway.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1194: Future<RawServerSocket> create() {
I would still prefer to just return a RawServerSocket.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1195: var port = new ReceivePort();
Consider using a RawReceivePort. It requires you to do more low-level stuff, but
it probably has lower overhead.
On the other hand, this function isn't likely to be performance critical.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1205: }
Consider adding:

  int get hashCode => _sendPort.hashCode;
  bool operator==(Object other) 
      => other is _RawServerSocketReference && 
         sendPort == other._sendPort;

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

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:340: * isolates. See [ServerSocketReference] for more
information.
ServerSocketReference doesn't really have more information.

How about:

The returned [ServerSocketReference] can be used to create other `ServerSocket`s
listening on the same port using [ServerSocketReference.create]. 
Incoming connections on the port will be distributed fairly between the active
server sockets.
The [ServerSocketReference] can be distributed to other isolates through a
[SendPort].

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:342: * All cloned [ServerSocket]s will receive incoming
[Socket]s using a
Will they get sockets before you listen on the ServerSocket?

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:343: * round-robin approach.
It's not round-robin unless they have a circular order, and each only gets one
event in each round. Is this the case?
Otherwise we need a different wording here.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:346: * Linux. The API is most likely going to change in
the near future.
I'd put "*" around "highly" too.

[Anders Johnsen, 2014-05-06 12:34:31]

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
File runtime/bin/eventhandler_linux.h (right):

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:43: } else {
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Add comment, either on method or here:
>  Adds entry just before head.

Done.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:47: e->next_->prev_ = e;
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Just:
>   head_->prev = e;

Done.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:70: void MoveNext() {
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Could this be named something with "rotate"?

Done.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/eventhandler...
runtime/bin/eventhandler_linux.h:163: return static_cast<uint32_t>(port &
0xFFFFFFFF);
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Why the '& 0xFFFFFFFF' ?
> Casting an int64_t to uint32_t should just pick the low 32 bits anyway.

Not sure, review feedback from sgjesse. Sgjesse?

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1090: ReceivePort _port;
On 2014/05/06 08:47:39, Søren Gjesse wrote:
> Call this variable something less generic, e.g. _referencePort.

Done.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1194: Future<RawServerSocket> create() {
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> I would still prefer to just return a RawServerSocket.

The problem is that it may return an invalid object. Let's iterate this one some
more.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1195: var port = new ReceivePort();
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Consider using a RawReceivePort. It requires you to do more low-level stuff,
but
> it probably has lower overhead.
> On the other hand, this function isn't likely to be performance critical.

Done.

https://codereview.chromium.org/250513002/diff/50001/runtime/bin/socket_patch...
runtime/bin/socket_patch.dart:1205: }
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Consider adding:
> 
>   int get hashCode => _sendPort.hashCode;
>   bool operator==(Object other) 
>       => other is _RawServerSocketReference && 
>          sendPort == other._sendPort;
> 

Done.

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

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:257: * WARNING: This feature is highly *experimental* and
currently only works on
On 2014/05/06 08:47:39, Søren Gjesse wrote:
> Move the WARNING up as the first line of the comment.

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:264: /**
On 2014/05/06 08:47:39, Søren Gjesse wrote:
> Add a WARNING here as well.
> 
> WARNING: This class is used with RawServerSocket.reference which is highly
> *experimental* ...

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:340: * isolates. See [ServerSocketReference] for more
information.
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> ServerSocketReference doesn't really have more information.
> 
> How about:
> 
> The returned [ServerSocketReference] can be used to create other
`ServerSocket`s
> listening on the same port using [ServerSocketReference.create]. 
> Incoming connections on the port will be distributed fairly between the active
> server sockets.
> The [ServerSocketReference] can be distributed to other isolates through a
> [SendPort].

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:342: * All cloned [ServerSocket]s will receive incoming
[Socket]s using a
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> Will they get sockets before you listen on the ServerSocket?

Yes

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:343: * round-robin approach.
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> It's not round-robin unless they have a circular order, and each only gets one
> event in each round. Is this the case?
> Otherwise we need a different wording here.

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:345: * WARNING: This feature is highly *experimental* and
currently only works on
On 2014/05/06 08:47:39, Søren Gjesse wrote:
> Move WARNING up.

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:346: * Linux. The API is most likely going to change in
the near future.
On 2014/05/06 09:04:20, Lasse Reichstein Nielsen wrote:
> I'd put "*" around "highly" too.

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:348: RawServerSocketReference get reference;
On 2014/05/06 08:47:39, Søren Gjesse wrote:
> Return type ServerSockerReference

Done.

https://codereview.chromium.org/250513002/diff/50001/sdk/lib/io/socket.dart#n...
sdk/lib/io/socket.dart:352: /**
On 2014/05/06 08:47:39, Søren Gjesse wrote:
> Add WARNING

Done.

[Anders Johnsen, 2014-05-06 19:09:52]

Committed patchset #5 manually as r35818 (presubmit successful).