Introduce and use Zone:_enter and Zone:_leave, in Future.

sdk/lib/async/future_impl.dart

Index: sdk/lib/async/future_impl.dart
diff --git a/sdk/lib/async/future_impl.dart b/sdk/lib/async/future_impl.dart
index 114614cae7304526360fcc983819139d4c12d67a..c9590d5a911e592d5cbf109ed2037b861401d63c 100644
--- a/sdk/lib/async/future_impl.dart
+++ b/sdk/lib/async/future_impl.dart
@@ -411,95 +411,87 @@ class _Future<T> implements Future<T> {
         _propagateMultipleListeners(source, listeners);
         return;
       }
-      if (hasError && !source._zone.inSameErrorZone(listener._zone)) {
+      Zone zone = listener._zone;
+      if (hasError && !source._zone.inSameErrorZone(zone)) {
         // Don't cross zone boundaries with errors.
         _AsyncError asyncError = source._error;
         source._zone.handleUncaughtError(
             asyncError.error, asyncError.stackTrace);
         return;
       }
-      if (!identical(Zone.current, listener._zone)) {
-        // Run the propagation in the listener's zone to avoid
-        // zone transitions. The idea is that many chained futures will
-        // be in the same zone.
-        listener._zone.run(() {
-          _propagateToListeners(source, listener);
-        });
-        return;
+      Zone old;

[floitsch, 2014/01/29 13:35:46]

Nit: I would prefer "oldZone", but "old" doesn't seem to conflict. so ok with
me.

[Anders Johnsen, 2014/01/29 15:30:33]

Done.

+      if (!identical(Zone.current, zone)) {
+        // Change zone if it's not current.
+        old = Zone._enter(zone);
       }
-
       // Do the actual propagation.
-      // TODO(floitsch): Do we need to go through the zone even if we
-      // don't have a callback to execute?
       bool listenerHasValue;
       var listenerValueOrError;
       // Set to true if a whenComplete needs to wait for a future.
       // The whenComplete action will resume the propagation by itself.
       bool isPropagationAborted = false;
-      // Even though we are already in the right zone (due to the optimization
-      // above), we still need to go through the zone. The overhead of
-      // executeCallback is however smaller when it is already in the correct
-      // zone.
-      // TODO(floitsch): only run callbacks in the zone, not the whole
-      // handling code.
-      listener._zone.run(() {
-        // TODO(floitsch): mark the listener as pending completion. Currently
-        // we can't do this, since the markPendingCompletion verifies that
-        // the future is not already marked (or chained).
+      // At this point we are in the right zone. Each callback is invoked

[floitsch, 2014/01/29 13:35:46]

You can just remove those two lines. I'm not sure they add anything.

[Anders Johnsen, 2014/01/29 15:30:33]

Done.

+      // through Zone.run* to be sure to invoke potential callbacks.
+      // TODO(floitsch): mark the listener as pending completion. Currently
+      // we can't do this, since the markPendingCompletion verifies that
+      // the future is not already marked (or chained).
+
+      bool handleValueCallback() {
         try {
-          if (!hasError) {
-            var value = source._value;
-            if (listener._onValue != null) {
-              listenerValueOrError = listener._onValue(value);
-              listenerHasValue = true;
-            } else {
-              // Copy over the value from the source.
-              listenerValueOrError = value;
-              listenerHasValue = true;
-            }
-          } else {
-            _AsyncError asyncError = source._error;
-            _FutureErrorTest test = listener._errorTest;
-            bool matchesTest = true;
-            if (test != null) {
-              matchesTest = test(asyncError.error);
-            }
-            if (matchesTest && listener._onError != null) {
-              Function errorCallback = listener._onError;
-              listenerValueOrError = _invokeErrorHandler(errorCallback,
-                                                         asyncError.error,
-                                                         asyncError.stackTrace);
-              listenerHasValue = true;
+          listenerValueOrError = zone.runUnary(listener._onValue,
+                                               source._value);
+          return true;
+        } catch (e, s) {
+          listenerValueOrError = new _AsyncError(e, s);
+          return false;
+        }
+      }
+
+      bool handleError() {
+        _AsyncError asyncError = source._error;
+        _FutureErrorTest test = listener._errorTest;
+        bool matchesTest = true;
+        if (test != null) {
+          try {
+            matchesTest = zone.runUnary(test, asyncError.error);
+          } catch (e, s) {
+            listenerValueOrError = identical(asyncError.error, e) ?

[floitsch, 2014/01/29 13:35:46]

I wonder if we shouldn't change the semantics here.
If the test throws, it should probably be considered as a real throw. Only the
catch-Error function should be able to "rethrow".

Maybe ask Lasse what he thinks.

[Anders Johnsen, 2014/01/29 15:30:33]

Adding TODO to be sure we revisit this.

+                asyncError : new _AsyncError(e, s);
+            listenerHasValue = false;
+            return false;
+          }
+        }
+        Function errorCallback = listener._onError;
+        if (matchesTest && errorCallback != null) {
+          try {
+            if (errorCallback is ZoneBinaryCallback) {
+              listenerValueOrError = zone.runBinary(errorCallback,
+                                                    asyncError.error,
+                                                    asyncError.stackTrace);
             } else {
-              // Copy over the error from the source.
-              listenerValueOrError = asyncError;
-              listenerHasValue = false;
+              listenerValueOrError = zone.runUnary(errorCallback,
+                                                   asyncError.error);
             }
+          } catch (e, s) {
+            listenerValueOrError = identical(asyncError.error, e) ?
+                asyncError : new _AsyncError(e, s);
+            listenerHasValue = false;
+            return false;
           }
+          listenerHasValue = true;
+        } else {
+          // Copy over the error from the source.
+          listenerValueOrError = asyncError;
+          listenerHasValue = false;
+        }
+        return true;
+      }
 
-          if (listener._whenCompleteAction != null) {
-            var completeResult = listener._whenCompleteAction();
-            if (completeResult is Future) {
-              listener._isChained = true;
-              completeResult.then((ignored) {
-                // Try again, but this time don't run the whenComplete callback.
-                _propagateToListeners(source, listener);
-              }, onError: (error, [stackTrace]) {
-                // When there is an error, we have to make the error the new
-                // result of the current listener.
-                if (completeResult is! _Future) {
-                  // This should be a rare case.
-                  completeResult = new _Future();
-                  completeResult._setError(error, stackTrace);
-                }
-                _propagateToListeners(completeResult, listener);
-              });
-              isPropagationAborted = true;
-            }
-          }
+      void handleWhenCompleteCallback() {
+        var completeResult;
+        try {
+          completeResult = zone.run(listener._whenCompleteAction);
         } catch (e, s) {
-          // Set the exception as error unless the error is the same as the
-          // original one.
           if (hasError && identical(source._error.error, e)) {
             listenerValueOrError = source._error;
           } else {
@@ -507,7 +499,42 @@ class _Future<T> implements Future<T> {
           }
           listenerHasValue = false;
         }
-      });
+        if (completeResult is Future) {
+          listener._isChained = true;
+          completeResult.then((ignored) {
+            // Try again, but this time don't run the whenComplete callback.

[floitsch, 2014/01/29 13:35:46]

// Try again. Since the future is marked as chained it won't run the
whenComplete again.

?

[Anders Johnsen, 2014/01/29 15:30:33]

Done.

+            _propagateToListeners(source, listener);
+          }, onError: (error, [stackTrace]) {
+            // When there is an error, we have to make the error the new
+            // result of the current listener.
+            if (completeResult is! _Future) {
+              // This should be a rare case.
+              completeResult = new _Future();
+              completeResult._setError(error, stackTrace);
+            }
+            _propagateToListeners(completeResult, listener);
+          });
+          isPropagationAborted = true;

[floitsch, 2014/01/29 13:35:46]

move to before the completeResult.then?

[Anders Johnsen, 2014/01/29 15:30:33]

Done.

+        }
+      }
+
+      if (!hasError) {
+        if (listener._onValue != null) {
+          listenerHasValue = handleValueCallback();
+        } else {
+          listenerValueOrError = source._value;
+          listenerHasValue = true;
+        }
+        if (listenerHasValue && listener._whenCompleteAction != null) {

[floitsch, 2014/01/29 13:35:46]

I don't think you need this.
The _whenCompleteAction and _onValue action can not happen at the same time.

[Anders Johnsen, 2014/01/29 15:30:33]

Done.

+          handleWhenCompleteCallback();

[floitsch, 2014/01/29 13:35:46]

It would be easier to optimize, if handleWhenCompleteCallback was only invoked
once. Then the compiler (or VM) could see that there is only one use, and just
inline it there (without code-bloat).

[Anders Johnsen, 2014/01/29 15:30:33]

Done.

+        }
+      } else {
+        if (handleError() && listener._whenCompleteAction != null) {
+          handleWhenCompleteCallback();
+        }
+      }
+      // If we changed zone, old will not be null.
+      if (old != null) Zone._leave(old);
       if (isPropagationAborted) return;
       // If the listener's value is a future we need to chain it.
       if (listenerHasValue && listenerValueOrError is Future) {