Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20703003:
Proposal for new Isolate library. (Closed)
|
Created:
7 years, 5 months ago by floitsch Modified:
7 years ago CC:
reviews_dartlang.org, Ben Laurie (Google) Visibility:
Public. |
DescriptionProposal for new Isolate library.
What this proposal tries to achieve:
- remove global port, stream.
- remove stream isolates. We want them in a package on top of the current API. (Potentially we can eventually move it back, but not now).
- Initiate the communication from the spawnee (using an initial message). If no communication is needed, then no Send/Receiveports are need.d
- provide arguments to a spawned isolate. These arguments are the same as the ones for command-line applications. -> it is possible to spawn command-line applications in the browser (as long as they don't use dart:io, ...).
- it also opens the door for more control on the spawned isolates. With this proposal the spawn functions are void. We can eventually make them return an object ("Isolate" ?) that can have properties on it. Good candidates (imho) would be stdin and stdout, and exit status code (much like the Process class).
Patch Set 1 #Patch Set 2 : Reupload due to error #Patch Set 3 : Forgot to save. #
Total comments: 13
Patch Set 4 : Address comments. #
Total comments: 13
Patch Set 5 : Comment changes. #Patch Set 6 : Update comment. #
Total comments: 2
Patch Set 7 : Using capabilities. #
Total comments: 30
Patch Set 8 : Address comments. #
Total comments: 12
Patch Set 9 : Address comments. #
Total comments: 10
Patch Set 10 : Address comments. #
Messages
Total messages: 17 (0 generated)
This is not intended for submission. Just for discussion.
I think this makes sense. Having no default ReceivePort in an isolate is a good
thing.
However setting up a duplex connection requires passing the spawned isolate to
at some point send a SendPort.
var isolatePort;
void isolateMain(message) {
message.send(new ReceivePort().toSendPort())
}
void isoladeReady() ...
void isolateMessage(message) ...
main() {
var port = new ReceivePort();
spawFunction(isolateMain, port.toSendPort();
port.receive((m) {
if (isolatePort == null) {
isolatePort = m;
iolateReady(m);
} else {
iolateMessage(m);
}
}
}
Maybe we should add some helper to set up the simple duplex communication using
two ports as part of dart:isolate.
The stream support could be put in dart:async instead of in a package.
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:22: dynamic get isolateArguments;
The name of this should be in the plural form. How about isolateMessage?
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:30: /**
Please remove or rewrite the old parts of the dartdoc.
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:43: void spawnFunction(void topLevelFunction(),
[dynamic message]);
Don't you want the topLevelFunction to take one argument?
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:45: /**
Ditto.
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:72: * serialization and deserialization should
work.
Should it be possible to override the built-in serialization as well?
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
sdk/lib/isolate/isolate.dart:84: void send(var message, { serializer(x),
SendPort replyTo });
Serializer and deserializer come i pairs. It could be nice if in the case of
spawnFunction these could (optionally) be specified when creating the
ReceivePort. The ReceivePort would then have a default serializer and all
derived SendPorts would have a deserializer.
On 2013/07/26 11:49:35, Søren Gjesse wrote:
> I think this makes sense. Having no default ReceivePort in an isolate is a
good
> thing.
>
> However setting up a duplex connection requires passing the spawned isolate to
> at some point send a SendPort.
>
> var isolatePort;
>
> void isolateMain(message) {
> message.send(new ReceivePort().toSendPort())
> }
>
> void isoladeReady() ...
> void isolateMessage(message) ...
>
> main() {
> var port = new ReceivePort();
> spawFunction(isolateMain, port.toSendPort();
> port.receive((m) {
> if (isolatePort == null) {
> isolatePort = m;
> iolateReady(m);
> } else {
> iolateMessage(m);
> }
> }
> }
>
> Maybe we should add some helper to set up the simple duplex communication
using
> two ports as part of dart:isolate.
I would have provided this in a package, but we could add it to dart:isolate if
we think that it's important.
>
> The stream support could be put in dart:async instead of in a package.
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.dart
> File sdk/lib/isolate/isolate.dart (right):
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
> sdk/lib/isolate/isolate.dart:22: dynamic get isolateArguments;
> The name of this should be in the plural form. How about isolateMessage?
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
> sdk/lib/isolate/isolate.dart:30: /**
> Please remove or rewrite the old parts of the dartdoc.
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
> sdk/lib/isolate/isolate.dart:43: void spawnFunction(void topLevelFunction(),
> [dynamic message]);
> Don't you want the topLevelFunction to take one argument?
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
> sdk/lib/isolate/isolate.dart:45: /**
> Ditto.
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
> sdk/lib/isolate/isolate.dart:72: * serialization and deserialization should
> work.
> Should it be possible to override the built-in serialization as well?
>
>
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da...
> sdk/lib/isolate/isolate.dart:84: void send(var message, { serializer(x),
> SendPort replyTo });
> Serializer and deserializer come i pairs. It could be nice if in the case of
> spawnFunction these could (optionally) be specified when creating the
> ReceivePort. The ReceivePort would then have a default serializer and all
> derived SendPorts would have a deserializer.
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:22: dynamic get isolateArguments; On 2013/07/26 11:49:35, Søren Gjesse wrote: > The name of this should be in the plural form. How about isolateMessage? Done. https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:30: /** On 2013/07/26 11:49:35, Søren Gjesse wrote: > Please remove or rewrite the old parts of the dartdoc. Done. https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:43: void spawnFunction(void topLevelFunction(), [dynamic message]); On 2013/07/26 11:49:35, Søren Gjesse wrote: > Don't you want the topLevelFunction to take one argument? Sounds interesting. Would you still set the `isolateMessage` ? Main argument against is consistency: the initial message is always in `isolateMessage`. https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:45: /** On 2013/07/26 11:49:35, Søren Gjesse wrote: > Ditto. Done. https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:72: * serialization and deserialization should work. On 2013/07/26 11:49:35, Søren Gjesse wrote: > Should it be possible to override the built-in serialization as well? Not sure. Could be an option, but it sounds dangerous. https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:84: void send(var message, { serializer(x), SendPort replyTo }); On 2013/07/26 11:49:35, Søren Gjesse wrote: > Serializer and deserializer come i pairs. It could be nice if in the case of > spawnFunction these could (optionally) be specified when creating the > ReceivePort. The ReceivePort would then have a default serializer and all > derived SendPorts would have a deserializer. I like this, but the problem is that you don't know where SendPorts end up. The other direction, giving a deserializer to the `send` could be easier. Added suggestion in comments.
https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/5001/sdk/lib/isolate/isolate.da... sdk/lib/isolate/isolate.dart:72: * serialization and deserialization should work. You can't override the serialization and deserialization of ports, because they are magically handled by the system, and you can't create one. You should not even be allowed to control the data that are passed to the deserializer because it would allow you to create bad ports. The isolate communication must be handled internally. We can then provide custom serialization/deserialization for user objects, but it will be on top of our own internal serialization. (And yes, I have ideas for this). https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:19: // are otherwise server-independent could be spawned in the browser. We should have an "Options" class somewhere for easy parsing of options, but it doesn't need to contain the arguments themselves. It can either get them as a list of strings, or default to using Isolate::isolateMessage. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:37: * spawnee can access it. Usually the initial [message] contains a [SendPort] so So the user is expected to use ports directly, and not streams based on ports? Or, in other words, Ports are still exposed? What are the restrictions on "message"? https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:44: * [spawnFunction], the child isolate will have a default [ReceivePort], and a It no longer returns a port. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:63: * The content of [message] can be: primitive values (null, num, bool, double, This is really annoying me, because I do it too: You list "null", which is a value, in a list of types. It would be more correct to write "Null", but it just reads so much better this way. Could we rename the type of "null" to "null"? (Probably too confusing, then we have to say "the null type" instead of just Null, but do we ever use Null in practice?) https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:67: * If, during serialization, an object of different type is encountered, the "an object of another type is". Using "different" requires saying different from what, and in which way. What you mean is "one not listed above". And which serialization? You haven't mentioned serialization before at all, so you need to start by saying that the message is serialized in some internal way, and only then say that you can have a custom serialization for unrecognized objects. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:69: * serialization and deserialization should work. That's an incredibly complex thing to just introduce in a side statement. That is, until we have something more to say, remove the parameter. We don't want a half-cocked serialization that users begin to use. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:125: * when receiving data. So the deserializer is invoked on the entire message, but the serializer only on the sub-parts that aren't handled? Drop this for now. We can make it symmetric when we have a good story about serialization.
https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:37: * spawnee can access it. Usually the initial [message] contains a [SendPort] so On 2013/08/12 07:19:03, Lasse Reichstein Nielsen wrote: > So the user is expected to use ports directly, and not streams based on ports? > Or, in other words, Ports are still exposed? Yes. But a good framework would abstract most of it away. > > What are the restrictions on "message"? Basically JSON + sendports. Not sure if we can add serializer support (like for SendPorts). https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:44: * [spawnFunction], the child isolate will have a default [ReceivePort], and a On 2013/08/12 07:19:03, Lasse Reichstein Nielsen wrote: > It no longer returns a port. Done. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:63: * The content of [message] can be: primitive values (null, num, bool, double, On 2013/08/12 07:19:03, Lasse Reichstein Nielsen wrote: > This is really annoying me, because I do it too: You list "null", which is a > value, in a list of types. It would be more correct to write "Null", but it just > reads so much better this way. > > Could we rename the type of "null" to "null"? (Probably too confusing, then we > have to say "the null type" instead of just Null, but do we ever use Null in > practice?) :) https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:67: * If, during serialization, an object of different type is encountered, the On 2013/08/12 07:19:03, Lasse Reichstein Nielsen wrote: > "an object of another type is". > > Using "different" requires saying different from what, and in which way. What > you mean is "one not listed above". done. > > And which serialization? You haven't mentioned serialization before at all, so > you need to start by saying that the message is serialized in some internal way, > and only then say that you can have a custom serialization for unrecognized > objects. > serialization is still vague. I need help to nail that part down. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:69: * serialization and deserialization should work. On 2013/08/12 07:19:03, Lasse Reichstein Nielsen wrote: > That's an incredibly complex thing to just introduce in a side statement. > That is, until we have something more to say, remove the parameter. We don't > want a half-cocked serialization that users begin to use. This is for discussion. I'm hoping we (devs) can come up with a good solution. At that point we must, of course, update this comment. https://codereview.chromium.org/20703003/diff/10001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:125: * when receiving data. On 2013/08/12 07:19:03, Lasse Reichstein Nielsen wrote: > So the deserializer is invoked on the entire message, but the serializer only on > the sub-parts that aren't handled? > Drop this for now. We can make it symmetric when we have a good story about > serialization. This CL is partially to discuss a good story. Leaving for now.
https://codereview.chromium.org/20703003/diff/22001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/22001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:22: dynamic get isolateMessage; Another global variable. Can we please just pass this as an argument to the function being called as "main"? If the isolate needs or expect arguments, it will have to have a function expecting them, which makes sense. https://codereview.chromium.org/20703003/diff/22001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:47: void spawnUri(Uri uri, [dynamic message]); Make it possible for this call to pass at least - current working directory - package root and any other external configurations necessary to run a script correctly. If nothing is passed, it should default to using the same values as this script.
PTAL. This patch set contains significant changes. - no serializer anymore. - the sendport of an isolate is exposed (and called "controlPort"). The receiving end of control ports are isolates and cannot be listened to by normal Dart programs. - capabilities: when sending a message to an isolate (through its control port) the corresponding capability is needed. - One can transfer control-ports, but in order to actually do something with the isolate one needs the capability. This means that the control-port can be used as identifier without leaking control. - ReceivePorts are now Stream<IsolateMessage> with IsolateMessage containing the replyPort and a convenient "reply" method.
After having gotten a crash-course on what Capability/Premission was I find this promising. Instead of isolates being "free floating" units of execution interconnected using ports there is some formalized structure on how they are controlled after creation. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:32: * other isolates. Maybe say that instances of Capability are opaque objects and from the instance itself it is not possible to see which permission on which isolate it grants. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:46: FULL_CONTROL premission? https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:77: * `main`. Otherwise similar to [spawn]. How about the arguments here vs. message above can you also send s SendPort here? If not then they are not that similar. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:108: * Pauses the isolate. How immediate is this pause? Pause when returning to the event loop? Assume that there can still be messages send from the isolate until the future completes. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:117: Future kill() { Should there be a optional Capability argument to pass a Capability obtained from somewhere else? https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:130: /// This id uniquely identifies the spawned isolate. "This id"? https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:131: final SendPort controlPort; I assume corresponding receive port is not directly visible but handled internally. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:136: * Clones this Isolate but with limited permissions. By "clone" don't you mean just making a new reference (controlPort) with limited capabilities. Not starting a new isolate. Shouldn't this return an Isolate?
https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:43: class Permission { Not sure you're going to need this class. It feels like an abstraction that really isn't needed -- could it be part of the Isolate class instead? https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:149: class IsolateMessage { I'm not too keen on wrapping all messages in new objects. Can we avoid that? I think we should send "raw" messages with associated ports/capabilities. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:191: void send(var message, { SendPort replyTo }); I think we should drop the replyTo argument (and let higher level abstraction add that if needed). Also, I like the idea of separating out the ports/capabilities into another list transmitted separately. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:200: * The [Future] extracts the message from the [IsolateMessage]. I don't like IsolateMessage in this context either. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:233: // I vote for non-broadcast. Non-broadcast gives us the option to add broadcasting on top of it at a higher level, right?
First set of comments. At some point I guess we would need an overview of the way you want to handle capabilities. Thanks, -Ivan https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:149: class IsolateMessage { On 2013/09/27 07:42:19, kasperl wrote: > I'm not too keen on wrapping all messages in new objects. Can we avoid that? I > think we should send "raw" messages with associated ports/capabilities. Agree with Kasper, no extra wrapping. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:191: void send(var message, { SendPort replyTo }); I do not see how the replyTo port is accessed on the receiving side. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:195: * Basically, this internally creates a new receive port, sends a This internal description is too detailed and we actually might want to change the implementation at some point as it is very wasteful with port allocation.
Thanks for all the feedback. I updated the CL. The isolate library becomes simpler. Missing: I still have to remove the "Permission" enum. It confuses readers, and the Permission class cannot be transferred as isolate message. I'm not yet sure how we want to deal with this. Maybe just a bunch of integer-constants in the Isolate class? https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:32: * other isolates. On 2013/09/27 07:04:11, Søren Gjesse wrote: > Maybe say that instances of Capability are opaque objects and from the instance > itself it is not possible to see which permission on which isolate it grants. Done. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:43: class Permission { On 2013/09/27 07:42:19, kasperl wrote: > Not sure you're going to need this class. It feels like an abstraction that > really isn't needed -- could it be part of the Isolate class instead? Yes, but I think there will be many Permissions. I will write another version that doesn't use permissions yet. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:46: On 2013/09/27 07:04:11, Søren Gjesse wrote: > FULL_CONTROL premission? Done. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:77: * `main`. Otherwise similar to [spawn]. On 2013/09/27 07:04:11, Søren Gjesse wrote: > How about the arguments here vs. message above can you also send s SendPort > here? If not then they are not that similar. Yes. It is crucial to be able to send Sendports. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:108: * Pauses the isolate. On 2013/09/27 07:04:11, Søren Gjesse wrote: > How immediate is this pause? Pause when returning to the event loop? Assume that > there can still be messages send from the isolate until the future completes. There will be different ways to pause: either at the end of the event-loop, or immediately. They will have different capabilities. This function was an example. When we actually implement it we will think about the names. Suggestions so far are: pause / suspend. (pause waits for event-loop). kill / terminate. (kill waits for event-loop). https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:117: Future kill() { On 2013/09/27 07:04:11, Søren Gjesse wrote: > Should there be a optional Capability argument to pass a Capability obtained > from somewhere else? Interesting idea. I will think about it. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:130: /// This id uniquely identifies the spawned isolate. On 2013/09/27 07:04:11, Søren Gjesse wrote: > "This id"? Old comment. Updated. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:131: final SendPort controlPort; On 2013/09/27 07:04:11, Søren Gjesse wrote: > I assume corresponding receive port is not directly visible but handled > internally. Correct. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:136: * Clones this Isolate but with limited permissions. On 2013/09/27 07:04:11, Søren Gjesse wrote: > By "clone" don't you mean just making a new reference (controlPort) with limited > capabilities. Not starting a new isolate. Correct. > > Shouldn't this return an Isolate? This is usually a way to send the permissions/capabilities to the other side (although "Permission" is currently not serializable). The result should therefore be a Map (or something serializable). I have to think about how to do this without the Permission class. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:149: class IsolateMessage { On 2013/09/27 07:42:19, kasperl wrote: > I'm not too keen on wrapping all messages in new objects. Can we avoid that? I > think we should send "raw" messages with associated ports/capabilities. Done. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:191: void send(var message, { SendPort replyTo }); On 2013/09/30 02:59:41, Ivan Posva wrote: > I do not see how the replyTo port is accessed on the receiving side. It would have been in the IsolateMessage. But gone now. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:195: * Basically, this internally creates a new receive port, sends a On 2013/09/30 02:59:41, Ivan Posva wrote: > This internal description is too detailed and we actually might want to change > the implementation at some point as it is very wasteful with port allocation. call is gone. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:200: * The [Future] extracts the message from the [IsolateMessage]. On 2013/09/27 07:42:19, kasperl wrote: > I don't like IsolateMessage in this context either. call is gone. https://codereview.chromium.org/20703003/diff/28001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:233: // I vote for non-broadcast. On 2013/09/27 07:42:19, kasperl wrote: > Non-broadcast gives us the option to add broadcasting on top of it at a higher > level, right? Non-broadcast it is.
https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:19: /** Do we need special Isolate-specific exceptions? Are they really that beneficial? https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:23: const IsolateSpawnException(String this._s); Remove String here. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:86: /** I'd prefer to just get rid of the (public) control port and permission stuff for now. I'd pick a single method to implement on Isolate and remove the rest. Maybe the nicest of the bunch is being able to listen for uncaught errors.
https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:22: class IsolateSpawnException implements Exception { Drop implementing Exception. It doesn't help anything anyway. What causes can there be for an Isolate not to be created? Can we enumerate them, and keep the reason in the exception, so the user knows what to do about it. If the user cannot do anything, consider making it an error. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:29: * This class should be part of dart:isolate. This sentence can be removed now. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:41: * An enum that enumeratos permissions that are required to control isolates. "enumerates"
https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:62: * takes no arguments. It is illegal to pass a function closure. illegal -> not allowed https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:64: * The entryPoint function is invoked with the initial [message]. `entryPoint` https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:66: * that the spawner and spawnee can communicate with each other. Document return value. https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:75: * The entry point of the spawned isolate is automatically set to "... that runs the code from the library with the specified URI. The isolate starts executing the top-level "main" function of the library with the given URI. Otherwise similar to [spawn]." (entryPoint is not specified in this method or the library declaration, it's just a name used by "spawn", and it doesn't sound like something you can "set" - the word "set" means too much in Dart to be used informally). https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:127: Future<bool> queryIsPaused() {} drop "query". We just have "isPaused" elsewhere, and the fact that it's async shouldn't change that.
https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:19: /** On 2013/10/14 06:58:12, kasperl wrote: > Do we need special Isolate-specific exceptions? Are they really that beneficial? I think it is good style to have specialized exceptions. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:22: class IsolateSpawnException implements Exception { On 2013/10/14 07:13:45, Lasse Reichstein Nielsen wrote: > Drop implementing Exception. It doesn't help anything anyway. > > What causes can there be for an Isolate not to be created? Can we enumerate > them, and keep the reason in the exception, so the user knows what to do about > it. I would like that yes. possible reasons: - url does not exist. - error during compilation of main (or reachable code). - out of resources (like too many running isolates). > > If the user cannot do anything, consider making it an error. in these cases it is an exceptional case. In other words: the user is using the API the right way and has no way to check that it won't throw an exception. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:23: const IsolateSpawnException(String this._s); On 2013/10/14 06:58:12, kasperl wrote: > Remove String here. Done. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:29: * This class should be part of dart:isolate. On 2013/10/14 07:13:45, Lasse Reichstein Nielsen wrote: > This sentence can be removed now. Done. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:41: * An enum that enumeratos permissions that are required to control isolates. On 2013/10/14 07:13:45, Lasse Reichstein Nielsen wrote: > "enumerates" Done. https://codereview.chromium.org/20703003/diff/35001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:86: /** On 2013/10/14 06:58:12, kasperl wrote: > I'd prefer to just get rid of the (public) control port and permission stuff for > now. > > I'd pick a single method to implement on Isolate and remove the rest. Maybe the > nicest of the bunch is being able to listen for uncaught errors. That would mean that there is no way to send an isolate to another isolate. https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.dart File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:62: * takes no arguments. It is illegal to pass a function closure. On 2013/10/14 14:26:48, Lasse Reichstein Nielsen wrote: > illegal -> not allowed Done. https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:64: * The entryPoint function is invoked with the initial [message]. On 2013/10/14 14:26:48, Lasse Reichstein Nielsen wrote: > `entryPoint` changed to "entry-point". https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:66: * that the spawner and spawnee can communicate with each other. On 2013/10/14 14:26:48, Lasse Reichstein Nielsen wrote: > Document return value. Done, although more needs to be done. https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:75: * The entry point of the spawned isolate is automatically set to On 2013/10/14 14:26:48, Lasse Reichstein Nielsen wrote: > "... that runs the code from the library with the specified URI. > > The isolate starts executing the top-level "main" function of the library with > the given URI. Otherwise similar to [spawn]." > > (entryPoint is not specified in this method or the library declaration, it's > just a name used by "spawn", and it doesn't sound like something you can "set" - > the word "set" means too much in Dart to be used informally). Done. https://codereview.chromium.org/20703003/diff/42001/sdk/lib/isolate/isolate.d... sdk/lib/isolate/isolate.dart:127: Future<bool> queryIsPaused() {} On 2013/10/14 14:26:48, Lasse Reichstein Nielsen wrote: > drop "query". We just have "isPaused" elsewhere, and the fact that it's async > shouldn't change that. Let's discuss this. If we call it "isPaused" it needs to be a getter, and I believe this would confuse users. While I agree that isPaused should not be a method, I (currently) prefer to use an action verb to make clear that the return value is not a bool. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
