Zone support for Futures.

Zone support for Futures.

R=lrn@google.com

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

[floitsch, 2013-05-16 18:37:32]

Still missing:
- Timer support.
- Stream changes.
- Tests.

[floitsch, 2013-05-17 09:32:56]

Renamed withErrors to catchErrors.
Reasoning: `withX` should change something for the scoped closure. For example:
withOutputToString(...)

[Lasse Reichstein Nielsen, 2013-05-21 08:19:57]

LGTM! STV!

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/future_impl....
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/future_impl....
sdk/lib/async/future_impl.dart:103: * This listener is installed at error-zone
switches. It guarantees that no
Need more explanation.
What is it installed on? By what? When is it called and by who?

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/future_impl....
sdk/lib/async/future_impl.dart:114: bool _inSameErrorZone(_Zone otherZone) {
Why is it not supported? Could it just return false?

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

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:9: throw "foo";
throw 0;
No need to introduce an string constant to the constant pool for this.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:15: dPrint(str) {} //=> print(str);
Remove this?
Name is bad in any case.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:32: final List<_Zone> _children = <_Zone>[];
If a zone can only be a member of one zone, then yes, they should form a
double-linked list (single linked, if removing isn't necessary).

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:41: assert(_parentZone != null || this is _DefaultZone);
You could make _DefaultZone implement _Zone instead of extending it, and not
have a special case here. It should probably special case most of the methods
anyway.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:72: // dPrint(_stackTrace);
Commented code.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:77: * Increments the open-callback counter.
Increments the number of open callbacks.

As long as there are open callbacks, the zone is running.

-- i.e., don't mention the counter. It's an implementation details. Just say
what it does. Don't use "is considered" - it either is or it is not, there can
be no "considered".

Consider renaming to: expectCallback(); or similar that doesn't mention the
"count". A rewording that doesn't even mention "increment" would be even better,
maybe just: 
"Expect one more callback".

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:84: * Decrements the open-callback counter.
Decrements the number of open callbacks.

When there are no more open callbacks, the zone is done and no more code will
run in the zone.

In most cases, it is recommended to run the callback through [executeCallback]
instead of manually calling this method.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:96: * Cleans up when the zone is done.
Cleans up this zone when it is done.

This releases internal memory structures that are no longer necessary.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:101: _onDone() {
Don't call it _onDone when it's not a generic overwritable _onDone handler (or
if it is, document it as such!).

Maybe call it _dispose()?

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:103: _parentZone._children.remove(this);
Ok, double-linked it is. And don't act on other object's properties directly,
add a _parentZone.removeChild(this) method.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:107: void _checkIfDone() {
Document when this is called.
"After any operation that finishes an expected callback, or when a child zone is
done".

I don't see a test after removing the child - maybe make a function to remove
the child, instead of directly calling parentZone._children.remove.
Or would it be a problem if the parent zone dies at the same time as the child
zone, when the child zone removes itself as the last child, because that might
make the dead parent zone the _current zone when the child returns?

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:109: dPrint("*/- done $this");
debug code.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:120: executeCallback(void fun()) {
void return type.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:129: executePeriodicCallback(void fun()) {
void return type. Check for more yourself.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:134: if (_current == this && _openCallbacks != 0) return
fun();
Consider using identical instead of ==.
Just in case users might get to implement Zone later.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:168: // TODO(floitsch): for debugging only. Should be
removed before committing.
Reminder to do todo.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:239: dPrint("WithError error: $error");
debug code.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:243: Future runWait(void fun()) {
Name is not very telling. I.e., I would have no idea what it does if I hadn't
been told.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:245: dPrint("closing");
debug code.

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
File tests/lib/async/catch_errors6_test.dart (right):

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
tests/lib/async/catch_errors6_test.dart:10: var port = new ReceivePort();
Document that the port keeps the test alive until it is closed, so a failure to
call port.close would make the test timeout and fail.

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
File tests/lib/async/catch_errors7_test.dart (right):

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
tests/lib/async/catch_errors7_test.dart:22: Expect.listEquals([
I prefer formatting this as either 
   ........ ["catch error entry,
             "main exit", 
             ...
             499],

or 
  ................. [
      "catch error entry",
      "main exit",
      ...
      499],

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
File tests/lib/async/catch_errors_test.dart (right):

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
tests/lib/async/catch_errors_test.dart:14: Expect.fail("Unexped callback");
Unexped -> Unexpected.

[floitsch, 2013-05-21 18:08:31]

PTAL.
I have split _Zone into an interface and a Base class.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/future_impl....
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/future_impl....
sdk/lib/async/future_impl.dart:103: * This listener is installed at error-zone
switches. It guarantees that no
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Need more explanation.
> What is it installed on? By what? When is it called and by who?

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/future_impl....
sdk/lib/async/future_impl.dart:114: bool _inSameErrorZone(_Zone otherZone) {
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Why is it not supported? Could it just return false?

It's basically an assert. It could return anything, but in theory it should
never be called.
I added a comment, but I can also change it to return true or false.

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

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:9: throw "foo";
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> throw 0;
> No need to introduce an string constant to the constant pool for this.

agreed.
Removed together with dPrint below.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:15: dPrint(str) {} //=> print(str);
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Remove this?
> Name is bad in any case.

Yes. was for debugging. Removed.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:32: final List<_Zone> _children = <_Zone>[];
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> If a zone can only be a member of one zone, then yes, they should form a
> double-linked list (single linked, if removing isn't necessary).

Kept TODO for now, but we should decide how we want this. Should the _Zone
interface mandate a linked-list interface, or should children get a token that
makes it easy to remove them?

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:41: assert(_parentZone != null || this is _DefaultZone);
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> You could make _DefaultZone implement _Zone instead of extending it, and not
> have a special case here. It should probably special case most of the methods
> anyway.

There is a lot of code that is the same (dealing with children...).
I added a "._default" constructor to _ZoneBase.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:72: // dPrint(_stackTrace);
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Commented code.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:77: * Increments the open-callback counter.
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Increments the number of open callbacks.
> 
> As long as there are open callbacks, the zone is running.
> 
> -- i.e., don't mention the counter. It's an implementation details. Just say
> what it does. Don't use "is considered" - it either is or it is not, there can
> be no "considered".
> 
> Consider renaming to: expectCallback(); or similar that doesn't mention the
> "count". A rewording that doesn't even mention "increment" would be even
better,
> maybe just: 
> "Expect one more callback".

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:84: * Decrements the open-callback counter.
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Decrements the number of open callbacks.
> 
> When there are no more open callbacks, the zone is done and no more code will
> run in the zone.
> 
> In most cases, it is recommended to run the callback through [executeCallback]
> instead of manually calling this method.

reworded. not exactly as you proposed, though.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:96: * Cleans up when the zone is done.
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Cleans up this zone when it is done.
> 
> This releases internal memory structures that are no longer necessary.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:101: _onDone() {
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Don't call it _onDone when it's not a generic overwritable _onDone handler (or
> if it is, document it as such!).
> 
> Maybe call it _dispose()?

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:103: _parentZone._children.remove(this);
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Ok, double-linked it is. And don't act on other object's properties directly,
> add a _parentZone.removeChild(this) method.

added _addChild and _removeChild.
Kept the list (and the TODO) for now.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:107: void _checkIfDone() {
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Document when this is called.
> "After any operation that finishes an expected callback, or when a child zone
is
> done".
> 
> I don't see a test after removing the child - maybe make a function to remove
> the child, instead of directly calling parentZone._children.remove.
> Or would it be a problem if the parent zone dies at the same time as the child
> zone, when the child zone removes itself as the last child, because that might
> make the dead parent zone the _current zone when the child returns?

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:109: dPrint("*/- done $this");
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> debug code.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:120: executeCallback(void fun()) {
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> void return type.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:129: executePeriodicCallback(void fun()) {
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> void return type. Check for more yourself.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:134: if (_current == this && _openCallbacks != 0) return
fun();
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Consider using identical instead of ==.
> Just in case users might get to implement Zone later.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:168: // TODO(floitsch): for debugging only. Should be
removed before committing.
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Reminder to do todo.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:239: dPrint("WithError error: $error");
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> debug code.

Done.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:243: Future runWait(void fun()) {
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Name is not very telling. I.e., I would have no idea what it does if I hadn't
> been told.

Changed to runWaitforCompletion.

https://codereview.chromium.org/14973006/diff/7001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:245: dPrint("closing");
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> debug code.

Done.

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
File tests/lib/async/catch_errors6_test.dart (right):

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
tests/lib/async/catch_errors6_test.dart:10: var port = new ReceivePort();
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Document that the port keeps the test alive until it is closed, so a failure
to
> call port.close would make the test timeout and fail.

Done.

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
File tests/lib/async/catch_errors7_test.dart (right):

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
tests/lib/async/catch_errors7_test.dart:22: Expect.listEquals([
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> I prefer formatting this as either 
>    ........ ["catch error entry,
>              "main exit", 
>              ...
>              499],
> 
> or 
>   ................. [
>       "catch error entry",
>       "main exit",
>       ...
>       499],
> 

I want to keep each event in a separate line so that it is easy to cut/paste.
Indended by 4/6 instead.
Not sure if that's better for you. let me know.

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
File tests/lib/async/catch_errors_test.dart (right):

https://codereview.chromium.org/14973006/diff/7001/tests/lib/async/catch_erro...
tests/lib/async/catch_errors_test.dart:14: Expect.fail("Unexped callback");
On 2013/05/21 08:19:57, Lasse Reichstein Nielsen wrote:
> Unexped -> Unexpected.

Done.

[Lasse Reichstein Nielsen, 2013-05-22 10:43:03]

lgtm

[dgrove, 2013-06-04 04:41:38]

Any idea when this will go in?

[floitsch, 2013-06-12 13:40:00]

Rebased and minor modifications (for example delaying the reporting of uncaught
errors).

[Lasse Reichstein Nielsen, 2013-06-13 11:46:15]

lgtm

[floitsch, 2013-06-14 16:27:01]

Committed patchset #6 manually as r24034 (presubmit successful).