SendPort + ReceivePort changes:

SendPort + ReceivePort changes:
- document what are valid messages in SendPort.send
- change SendPort.call to return a future of the reply
- remove ReceivePort.singleShot

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

[Siggi Cherem (dart-lang), 2012-03-08 21:57:41]

These are changes that we've discussed so far in the new proposed API.

Ivan and Kasper - I'd like to make sure you are OK with the API change. 

Mads - I want to make sure the changes to dart:io are ok. 

Most uses of sendport.call don't use the reply port (except when we did this on
purpose for a test).

ReceivePort.singleShot was mainly used in .call, we use it as an adhoc trick for
ensuring that the vm waits for another isolate in dart:io, but this is such a
specific use case, I'm not sure it makes it worth having singleShot as part of
the general API.

[Mads Ager (google), 2012-03-09 10:30:06]

The dart:io changes look good for the new API.

I'm not sure that I like spreading the use of Future in the isolate API. I think
I would rather go the other way and eliminate it from spawn:

isolate.spawn((port) {
  // do stuff
});

However, I have not been involved in the API discussions so feel free to ignore.
:)

[kasperl, 2012-03-09 10:36:46]

LGTM. I'm okay with removing ReceivePort.singleShot for now even if it ends up
coming back in some form.

[Siggi Cherem (dart-lang), 2012-03-09 17:49:36]

On 2012/03/09 10:30:06, Mads Ager wrote:
> The dart:io changes look good for the new API.
> 
> I'm not sure that I like spreading the use of Future in the isolate API. I
think
> I would rather go the other way and eliminate it from spawn:
> 
> isolate.spawn((port) {
>   // do stuff
> });
> 
> However, I have not been involved in the API discussions so feel free to
ignore.
> :)

Hi Mads,

Thanks for the comments. I actually agree with you too :) - we are eliminating
future where it felt unnecessary. For instance, the API that used to be
"Future<SendPort> futPort = new IsolateSubclass().spawn()"  is changing to a new
API (which is now implemented in frog) that looks more like this: "SendPort port
= spawn(f);"

In the case of the change in this CL, [SendPort.call] needed a callback with
possibly an error callback too, which is what Futures are designed for.

[eub, 2012-03-09 18:33:10]

LGTM, one change to consider (which you may have considered already).

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/frog/isolateimpl...
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:298: port.close();
I wonder how often people are going to write this idiom.  Maybe
SingleShotReceivePort is worth still having around?

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:298: port.close();
I saw about a dozen instances looking over this CL.

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/isolate_api.dart
File lib/isolate/isolate_api.dart (right):

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/isolate_api.dart...
lib/isolate/isolate_api.dart:81: Future call(var message);
♥

http://codereview.chromium.org/9652001/diff/5001/tests/isolate/src/TestFramew...
File tests/isolate/src/TestFramework.dart (right):

http://codereview.chromium.org/9652001/diff/5001/tests/isolate/src/TestFramew...
tests/isolate/src/TestFramework.dart:251: final port = new ReceivePort();
I'm missing why the port vs. keepalive distinction here.

[Ivan Posva, 2012-03-09 19:38:09]

LGTM.

The single shot receive ports always looked like a special case to me.

-Ivan

https://chromiumcodereview.appspot.com/9652001/diff/5001/lib/isolate/isolate_...
File lib/isolate/isolate_api.dart (right):

https://chromiumcodereview.appspot.com/9652001/diff/5001/lib/isolate/isolate_...
lib/isolate/isolate_api.dart:118: void receive(void callback(var message,
SendPort replyTo));
Not necessarily this change, but we should really change this to onReceive. This
would be inline with the rest of the asynchronous Dart APIs.

[Siggi Cherem (dart-lang), 2012-03-10 03:09:59]

Thanks for all the comments!

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/frog/isolateimpl...
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:298: port.close();
On 2012/03/09 18:33:10, eub wrote:
> I saw about a dozen instances looking over this CL.

Yeah - this is something that I saw commonly in tests and places where the
parent isolate wanted to 'wait' for the child isolate before the vm finishes
executing. The trick is to keep a port open, so that the vm knows that we are
not done. 

Using singleShot for this purpose made the idiom shorter to write, but a little
harder to see in my opinion.

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/isolate_api.dart
File lib/isolate/isolate_api.dart (right):

http://codereview.chromium.org/9652001/diff/5001/lib/isolate/isolate_api.dart...
lib/isolate/isolate_api.dart:81: Future call(var message);
On 2012/03/09 18:33:10, eub wrote:
> ♥

:)

http://codereview.chromium.org/9652001/diff/5001/tests/isolate/src/TestFramew...
File tests/isolate/src/TestFramework.dart (right):

http://codereview.chromium.org/9652001/diff/5001/tests/isolate/src/TestFramew...
tests/isolate/src/TestFramework.dart:251: final port = new ReceivePort();
On 2012/03/09 18:33:10, eub wrote:
> I'm missing why the port vs. keepalive distinction here.

It happens that keepalive is a field that gets assigned to null before we can
close it (line 263). We just needed a local copy that is still accesible in the
callback of [receive]

[turnidge, 2012-03-12 23:03:20]

lgtm