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

sdk/lib/async/future_impl.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of dart.async;
 
 /** The onValue and onError handlers return either a value or a future */
 typedef dynamic _FutureOnValue<T>(T value);
 /** Test used by [Future.catchError] to handle skip some errors. */
 typedef bool _FutureErrorTest(var error);
@@ skipping 393 matching lines @@
         return;
       }
       if (listeners == null) return;
       _Future listener = listeners;
       if (listener._nextListener != null) {
         // Usually futures only have one listener. If they have several, we
         // handle them specially.
         _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;
+          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 value from the source.
-              listenerValueOrError = value;
-              listenerHasValue = true;
+              listenerValueOrError = zone.runUnary(errorCallback,
+                                                   asyncError.error);
             }
-          } 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;
-            } else {
-              // Copy over the error from the source.
-              listenerValueOrError = asyncError;
-              listenerHasValue = false;
-            }
+          } 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 {
             listenerValueOrError = new _AsyncError(e, s);
           }
           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) {
         Future chainSource = listenerValueOrError;
         // Shortcut if the chain-source is already completed. Just continue the
         // loop.
         if (chainSource is _Future && chainSource._isComplete) {
           // propagate the value (simulating a tail call).
           listener._isChained = true;
           source = chainSource;
@@ skipping 44 matching lines @@
       }
     }, onError: (e, s) {
       if (timer.isActive) {
         timer.cancel();
         result._completeError(e, s);
       }
     });
     return result;
   }
 }