Add tasks to zones.

Add tasks to zones.

R=lrn@google.com, misko@google.com

Committed: https://github.com/dart-lang/sdk/commit/85cccde71792a0e2920bd8e2d4ebf73880355940

[floitsch, 2016-03-31 19:18:00]

floitsch@google.com changed reviewers:
+ lrn@google.com, misko@google.com, ralphj@google.com

[floitsch, 2016-03-31 19:18:01]

Next attempt.

The idea is that every operation that is triggered by the event-loop goes
through the zone in the following way:

1. Task Zone.createTask(TaskSpecification, create)
  This (interceptable) method creates the task. This was called 'schedule'
before.
  The 'create' argument is the one that is called by the zones, once they have
done their intercepting.
2. void Zone.runTask(Task, run)
  This (interceptable) method runs the task. The 'run' argument is the one doing
the actual 'running', once the zones have done their intercepting.

As before there is also a `cancelTask` which works very similarly to `runTask`.


Note: this doesn't yet contain the changes needed for the HTML library. I hope
they won't be too much work, but I'm not sure.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:74: Zone get zone;
We could remove this field.

[Lasse Reichstein Nielsen, 2016-04-01 21:26:35]

It makes much more sense to me now. I'll still quibble about details, obviously
:)

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:11: typedef Task TaskCreate(TaskSpecification
taskSpecification, Zone zone);
Which zone are we passing to the create function?

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:74: Zone get zone;
Agree. I generally prefer not to leak zones, so code can only run in the zone it
is started it, that it creates itself, or that it has deliberately been granted
access to by passing it a zone explicitly. It's better for encapsulation.
(And Zone.root should be private :)

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:87: /// they may never run, or run multiple times.
That's rather the absence of specific expectations - something expected to run
exactly once, or at most once, is more specific.

If an event is [isMacroTask] and ![isOneShot], is it then always an event task?
If not, what distinguishes event tasks from that other task? If so, the
[isEventTask] is redundant (might still be useful as a shorthand, though).

All in all, I don't see "eventTask" as useful without a firmer specification of
what it is and isn't (and then only if it's still useful).

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:96: /// Macro tasks are triggered from the event loop
and start a new micro task
"triggered from the event loop" is vague (as in: I still can't pinpoint what it
means, especially the "triggered" word).
It really is just "events" - that's why it's called "the event loop".

How about:

 Macro tasks are tasks executed by the main event loop. Each such task
 may schedule a number of microtasks to be executed before the next 
 macro task.
 Macro tasks include tasks triggered by timer events, system events
 (like I/O or DOM events) and port message events.

(Hmm, I used "triggered" and it made sense to me, so my problem was probably
that I don't see the event loop as triggering anything, external events trigger
something, it's then executed by the event loop).

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:102: /// If the task is not a one-shot task, it must be
canceled.
"must be canceled" may be too strong. You can probably also just ignore some
tasks if you know they won't generate more events. Maybe just "may need to be
cancelled to prevent further iterations of the task".

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:148: String get description => "Timer";
Maybe just make this the "toString" of the task-spec instead of having a
description.
You shouldn't be parsing strings anyway, and you need to do that to see that
"Periodic Timer" is a timer. You can't prevent anybody from using "is
PeridocTimerTaskSpecification" either, so any mocking will need to implement the
interface anyway.

That is: if description is solving any problem, then a string isn't the best
solution. If it's just for readability, make it the toString.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:163: bool get isMacroTask => true;
I still prefer putting getters after the constructor instead of with the fields
(probably because I want to see structure when reading the source of a class,
not API - I'll read the API docs for that).

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:220: TaskSpecification taskSpecification, TaskCreate
create),
I'm still not sure I think the create argument is necessary.
It *feels* redundant.

If the root zone recognizes the system task specifications, then it can create
tasks. If you need to add more task types to the system, then you need to run in
a zone that recognize those specifications.

If you need to replace the create function, then just call that new function
directly and don't call parent.createTask.
This is similar to DOM events where you can stop the propagation.

When you also pass the create function, and you are allowed to replace it, e.g.,
by wrapping the original function, then you can interleave yourself into the
propagation in two ways:

   self.createTask before calling parent.createTask
   parent.createTask before running wrapped create
   self's wrapped create before calling original create
   original create
   self's wrapped create after calling original create 
   parent.createTask after running wrapped create
   self.createTask after calling parent.createTask

This allows the "self" zone to inject itself everywhere. I think that's too much
to reason about - if it was just the outer wrapping, and either recurse to
parent or replace the parent behavior, then it's easier to reason about.

That doesn't allow you to add unknown task types to the root - the zone needs to
know about the task type or it won't be recognized.

[floitsch, 2016-04-05 20:07:17]

One comment I strongly disagree, and one that we don't agree yet, but otherwise
we are converging!

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:11: typedef Task TaskCreate(TaskSpecification
taskSpecification, Zone zone);
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> Which zone are we passing to the create function?

The zone in which the task should execute, when there is a 'runTask'.
Basically:
zone.createTask(..) calls the delegates with the zone as argument. Each delegate
may change the zone (although it's discouraged). Eventually, the delegate-chain
reaches the root zone (or not) which invokes the 'create' function, giving the
'zone' as argument.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:74: Zone get zone;
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> Agree. I generally prefer not to leak zones, so code can only run in the zone
it
> is started it, that it creates itself, or that it has deliberately been
granted
> access to by passing it a zone explicitly. It's better for encapsulation.

The thing is, that the task needs to keep track of in which zone it has to run.
However, I noticed, that I just captured the zone in a closure anyway. Still:
the task will know in which zone it has to run.

> (And Zone.root should be private :)
That ship has sailed :)

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:87: /// they may never run, or run multiple times.
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> That's rather the absence of specific expectations - something expected to run
> exactly once, or at most once, is more specific.
> 
> If an event is [isMacroTask] and ![isOneShot], is it then always an event
task?
> If not, what distinguishes event tasks from that other task? If so, the
> [isEventTask] is redundant (might still be useful as a shorthand, though).
> 
> All in all, I don't see "eventTask" as useful without a firmer specification
of
> what it is and isn't (and then only if it's still useful).

Fair enough. It might be more interesting to have a predicate for the more
regular tasks. I will ask Misko, what property exactly they want from
EventTasks. Personally, I don't think it matters much.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:96: /// Macro tasks are triggered from the event loop
and start a new micro task
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> "triggered from the event loop" is vague (as in: I still can't pinpoint what
it
> means, especially the "triggered" word).
> It really is just "events" - that's why it's called "the event loop".
> 
> How about:
> 
>  Macro tasks are tasks executed by the main event loop. Each such task
>  may schedule a number of microtasks to be executed before the next 
>  macro task.
>  Macro tasks include tasks triggered by timer events, system events
>  (like I/O or DOM events) and port message events.
> 
> (Hmm, I used "triggered" and it made sense to me, so my problem was probably
> that I don't see the event loop as triggering anything, external events
trigger
> something, it's then executed by the event loop).

Done.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:102: /// If the task is not a one-shot task, it must be
canceled.
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> "must be canceled" may be too strong. You can probably also just ignore some
> tasks if you know they won't generate more events. Maybe just "may need to be
> cancelled to prevent further iterations of the task".

Done.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:148: String get description => "Timer";
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> Maybe just make this the "toString" of the task-spec instead of having a
> description.
> You shouldn't be parsing strings anyway, and you need to do that to see that
> "Periodic Timer" is a timer. You can't prevent anybody from using "is
> PeridocTimerTaskSpecification" either, so any mocking will need to implement
the
> interface anyway.
> 
> That is: if description is solving any problem, then a string isn't the best
> solution. If it's just for readability, make it the toString.

This is the identifier that delegates use to intercept task creations.
Ideally, we would like to have unique ids, but that's very hard to enforce.
Instead, we are relying on users to follow conventions and not step on each
other's toes.
I agree, that one could simply use the type of the TaskSpecification, but I find
strings nicer in this use case. If we go for 'toString', then there isn't any
difference to a 'description' field anyway. It would just be easier to make a
mistake (since the description field is very explicit).

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:163: bool get isMacroTask => true;
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> I still prefer putting getters after the constructor instead of with the
fields
> (probably because I want to see structure when reading the source of a class,
> not API - I'll read the API docs for that).

Done.

https://codereview.chromium.org/1848933002/diff/1/sdk/lib/async/zone.dart#new...
sdk/lib/async/zone.dart:220: TaskSpecification taskSpecification, TaskCreate
create),
On 2016/04/01 21:26:35, Lasse Reichstein Nielsen wrote:
> I'm still not sure I think the create argument is necessary.
> It *feels* redundant.
> 
> If the root zone recognizes the system task specifications, then it can create
> tasks. If you need to add more task types to the system, then you need to run
in
> a zone that recognize those specifications.
I strongly disagree.
The root-zone must be able to deal with html-event tasks, even though it has no
idea that the html library even exists.
Only, zones that want to intercept task creations need to know about specific
tasks (and even then, they might not be able to actually create the task).

> 
> If you need to replace the create function, then just call that new function
> directly and don't call parent.createTask.
You could do that (although I would find it bad style).
However, you still need to handle the standard root-zone case.

> This is similar to DOM events where you can stop the propagation.

> 
> When you also pass the create function, and you are allowed to replace it,
e.g.,
> by wrapping the original function, then you can interleave yourself into the
> propagation in two ways:
> 
>    self.createTask before calling parent.createTask
>    parent.createTask before running wrapped create
>    self's wrapped create before calling original create
>    original create
>    self's wrapped create after calling original create 
>    parent.createTask after running wrapped create
>    self.createTask after calling parent.createTask
> 
> This allows the "self" zone to inject itself everywhere. I think that's too
much
> to reason about - if it was just the outer wrapping, and either recurse to
> parent or replace the parent behavior, then it's easier to reason about.
I don't think it would be common to wrap the 'create' function. In
testing-zones, for example, one would simply replace the create function, with a
mock-function.

> 
> That doesn't allow you to add unknown task types to the root - the zone needs
to
> know about the task type or it won't be recognized.
I don't see how this would be possible. The root-zone must be able to deal with
all macro tasks, but doesn't know them yet: the html-library was added after the
fact. Similarly, mojo and flutter introduce their own macrotasks...

[floitsch, 2016-05-17 20:20:25]

PTAL.

[misko, 2016-05-17 22:28:49]

On 2016/05/17 20:20:25, floitsch wrote:
> PTAL.

LGTM

[Lasse Reichstein Nielsen, 2016-05-18 11:40:55]

LGTM, but I still want to see that performance doesn't suffer too much.

https://codereview.chromium.org/1848933002/diff/60001/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1848933002/diff/60001/sdk/lib/async/zone.dart...
sdk/lib/async/zone.dart:31: TaskSpecification taskSpecification, TaskCreate
schedule);
schedule -> create.

https://codereview.chromium.org/1848933002/diff/60001/sdk/lib/async/zone.dart...
sdk/lib/async/zone.dart:91: abstract class TimerTask {
Maybe document the class - where is it used, what can it be used for.

https://codereview.chromium.org/1848933002/diff/60001/sdk/lib/async/zone.dart...
sdk/lib/async/zone.dart:99: class SingleShotTimerTask extends TimerTask {
Consider moving the timer-specific classes to the same file as the timer
implementation.

[Lasse Reichstein Nielsen, 2016-05-20 22:01:36]

https://codereview.chromium.org/1848933002/diff/80001/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/1848933002/diff/80001/sdk/lib/async/zone.dart...
sdk/lib/async/zone.dart:35: Object task, Object arg1, TaskRun run);
Just noticed: I'd swap the run function and the argument.

In runUnary and runBinary we pass the function before its arguments, this should
be consistent.

[floitsch, 2016-05-31 19:23:49]

Changed the orders.
If you want PTAL.

[floitsch, 2016-06-06 20:22:05]

I implemented task support for the HTML library (events, requestAnimationFrame,
and http-requests).
As a result this CL has changed slightly. There is no `cancelTask` anymore.

[floitsch, 2016-06-06 20:22:33]

Description was changed from

==========
Evolution of 'separate task and specification'.
==========

to

==========
Add tasks to zones.
==========

[Lasse Reichstein Nielsen, 2016-07-01 11:39:12]

lgtm

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
File sdk/lib/async/zone.dart (right):

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:61: // non-experimental.
remove comma.

This is not a dart-doc, so it doesn't document a declaration. It covers both the
following typedefs, which isn't clear.

Maybe:
 The following typedef declarations are used by functionality 
 which will be removed and replaced by tasks 
 if the task experiment is successful.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:518: * through the event loop. For example, a timer or
an http request both
Still not sure I like "operation that returns through the event loop" - it's
unclear what that means.
So, maybe, "operation that will later generate one or more events through the
event loop."?

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:533: * This function is invoked, when an operation,
started through [createTask],
remove comma before "when".

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:534: * returns to Dart code.
"returns to Dart code" -> "generates an event".

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:536: * Generally, tasks schedule Dart code in the global
event loop. As such,
I think something is missing here, but I can't find something better to say. I
want a "when ..." after the "global event loop", but I can't actually say when.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:541: * It is good practice, if task operations provide a
meaningful [arg], so
, if -> that

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:542: * that custom zones can deal with it. They might
want to log it, or
maybe:
can deal with it -> can see what is happening

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:542: * that custom zones can deal with it. They might
want to log it, or
it, or -> or

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:543: * replace it.
it -> the argument before calling the [run] function.

[floitsch, 2016-07-02 00:42:43]

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
File sdk/lib/async/zone.dart (right):

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:61: // non-experimental.
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> remove comma.
> 
> This is not a dart-doc, so it doesn't document a declaration. It covers both
the
> following typedefs, which isn't clear.
> 
> Maybe:
>  The following typedef declarations are used by functionality 
>  which will be removed and replaced by tasks 
>  if the task experiment is successful.

Done.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:518: * through the event loop. For example, a timer or
an http request both
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> Still not sure I like "operation that returns through the event loop" - it's
> unclear what that means.
> So, maybe, "operation that will later generate one or more events through the
> event loop."?

Sorry for the confusion. I had a follow-up CL to improve the documentation. I
now copied the updated documentation in this CL.

This was already fixed in the doc-update.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:533: * This function is invoked, when an operation,
started through [createTask],
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> remove comma before "when".

Was already fixed in the other CL. Again copied it here now.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:534: * returns to Dart code.
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> "returns to Dart code" -> "generates an event".

Done.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:536: * Generally, tasks schedule Dart code in the global
event loop. As such,
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> I think something is missing here, but I can't find something better to say. I
> want a "when ..." after the "global event loop", but I can't actually say
when.

Done.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:541: * It is good practice, if task operations provide a
meaningful [arg], so
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> , if -> that

Done.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:542: * that custom zones can deal with it. They might
want to log it, or
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> maybe:
> can deal with it -> can see what is happening

changed to 'interact'.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:542: * that custom zones can deal with it. They might
want to log it, or
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> it, or -> or

Done.

https://chromiumcodereview.appspot.com/1848933002/diff/200001/sdk/lib/async/z...
sdk/lib/async/zone.dart:543: * replace it.
On 2016/07/01 11:39:11, Lasse Reichstein Nielsen wrote:
> it -> the argument before calling the [run] function.

Done.

[floitsch, 2016-07-02 02:42:24]

Description was changed from

==========
Add tasks to zones.
==========

to

==========
Add tasks to zones.

R=lrn@google.com, misko@google.com

Committed:
https://github.com/dart-lang/sdk/commit/85cccde71792a0e2920bd8e2d4ebf73880355940
==========

[floitsch, 2016-07-02 02:42:26]

Committed patchset #13 (id:240001) manually as
85cccde71792a0e2920bd8e2d4ebf73880355940 (presubmit successful).