Simplify future-propagation for whenComplete.

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 169 matching lines @@
 
   /// Valid types for value: `T` or `Future<T>`.
   _Future.immediate(value) {
     _asyncComplete(value);
   }
 
   _Future.immediateError(var error, [StackTrace stackTrace]) {
     _asyncCompleteError(error, stackTrace);
   }
 
-  bool get _mayComplete => _state == _INCOMPLETE;
-  bool get _isChained => _state == _CHAINED;
-  bool get _isComplete => _state >= _VALUE;
-  bool get _hasValue => _state == _VALUE;
-  bool get _hasError => _state == _ERROR;
+  bool get _mayComplete       => _state == _INCOMPLETE;
+  bool get _isPendingComplete => _state == _PENDING_COMPLETE;
+  bool get _isChained         => _state == _CHAINED;
+  bool get _isComplete        => _state >= _VALUE;
+  bool get _hasValue          => _state == _VALUE;
+  bool get _hasError          => _state == _ERROR;
 
-  set _isChained(bool value) {
-    if (value) {
-      assert(!_isComplete);
-      _state = _CHAINED;
-    } else {
-      assert(_isChained);
-      _state = _INCOMPLETE;
-    }
+  void _setChained() {
+    assert(!_isComplete);
+    _state = _CHAINED;
   }
 
   Future then(f(T value), { Function onError }) {
     Zone currentZone = Zone.current;
     if (!identical(currentZone, _ROOT_ZONE)) {
       f = currentZone.registerUnaryCallback(f);
       if (onError != null) {
         onError = _registerErrorHandler(onError, currentZone);
       }
     }
@@ skipping 83 matching lines @@
     while (current != null) {
       _FutureListener next = current._nextListener;
       current._nextListener = prev;
       prev = current;
       current = next;
     }
     return prev;
   }
 
   // Take the value (when completed) of source and complete target with that
-  // value (or error). This function can chain all Futures, but is slower
-  // for _Future than _chainCoreFuture - Use _chainCoreFuture in that case.
+  // value (or error). This function could chain all Futures, but is slower
+  // for _Future than _chainCoreFuture, so you must use _chainCoreFuture
+  // in that case.
   static void _chainForeignFuture(Future source, _Future target) {
     assert(!target._isComplete);
     assert(source is! _Future);
 
     // Mark the target as chained (and as such half-completed).
-    target._isChained = true;
+    target._setChained();
     try {
       source.then((value) {
           assert(target._isChained);
           target._completeWithValue(value);
         },
         // TODO(floitsch): eventually we would like to make this non-optional
         // and dependent on the listeners of the target future. If none of
         // the target future's listeners want to have the stack trace we don't
         // need a trace.
         onError: (error, [stackTrace]) {
@@ skipping 11 matching lines @@
     }
   }
 
   // Take the value (when completed) of source and complete target with that
   // value (or error). This function expects that source is a _Future.
   static void _chainCoreFuture(_Future source, _Future target) {
     assert(!target._isComplete);
     assert(source is _Future);
 
     // Mark the target as chained (and as such half-completed).
-    target._isChained = true;
+    target._setChained();
     _FutureListener listener = new _FutureListener.chain(target);
     if (source._isComplete) {
       _propagateToListeners(source, listener);
     } else {
       source._addListener(listener);
     }
   }
 
   void _complete(value) {
     assert(!_isComplete);
@@ skipping 40 matching lines @@
     // unhandled error, even though we know we are already going to listen to
     // it.
 
     if (value == null) {
       // No checks for `null`.
     } else if (value is Future) {
       // Assign to typed variables so we get earlier checks in checked mode.
       Future<T> typedFuture = value;
       if (typedFuture is _Future) {
         _Future<T> coreFuture = typedFuture;
-        if (coreFuture._isComplete && coreFuture._hasError) {
+        if (coreFuture._hasError) {
           // Case 1 from above. Delay completion to enable the user to register
           // callbacks.
           _markPendingCompletion();
           _zone.scheduleMicrotask(() {
             _chainCoreFuture(coreFuture, this);
           });
         } else {
           _chainCoreFuture(coreFuture, this);
         }
       } else {
@@ skipping 42 matching lines @@
       // call handle them separately in recursive calls, continuing
       // here only when there is only one listener left.
       while (listeners._nextListener != null) {
         _FutureListener listener = listeners;
         listeners = listener._nextListener;
         listener._nextListener = null;
         _propagateToListeners(source, listener);
       }
       _FutureListener listener = listeners;
       // Do the actual propagation.
-      // Set initial state of listenerHasValue and listenerValueOrError. These
-      // variables are updated, with the outcome of potential callbacks.
-      bool listenerHasValue = true;
-      final sourceValue = hasError ? null : source._value;
-      var listenerValueOrError = sourceValue;
-      // Set to true if a whenComplete needs to wait for a future.
-      // The whenComplete action will resume the propagation by itself.
-      bool isPropagationAborted = false;
-      // 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).
+      // Set initial state of listenerHasError and listenerValueOrError. These
+      // variables are updated with the outcome of potential callbacks.
+      bool listenerHasError = hasError;
+      final sourceResult = source._resultOrListeners;
+      var listenerValueOrError = sourceResult;
+
       // Only if we either have an error or callbacks, go into this, somewhat
       // expensive, branch. Here we'll enter/leave the zone. Many futures
       // doesn't have callbacks, so this is a significant optimization.

[floitsch, 2015/12/09 22:52:01]

don't

-      if (hasError || (listener.handlesValue || listener.handlesComplete)) {
+      if (hasError || listener.handlesValue || listener.handlesComplete) {
         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;
         }
 
         Zone oldZone;
         if (!identical(Zone.current, zone)) {
           // Change zone if it's not current.
           oldZone = Zone._enter(zone);
         }
 
         bool handleValueCallback() {

[floitsch, 2015/12/09 22:52:01]

doesn't return anything anymore.

[Lasse Reichstein Nielsen, 2015/12/10 06:52:26]

Done.

+          assert(!hasError);
           try {
             listenerValueOrError = zone.runUnary(listener._onValue,
-                                                 sourceValue);
-            return true;
+                                                 sourceResult);
+            listenerHasError = false;
           } catch (e, s) {
             listenerValueOrError = new AsyncError(e, s);
-            return false;
+            listenerHasError = true;
           }
         }
 
-        void handleError() {
+        bool handleError() {

[floitsch, 2015/12/09 22:52:02]

doesn't return anything anymore.

[Lasse Reichstein Nielsen, 2015/12/10 06:52:26]

Done.

           AsyncError asyncError = source._error;
           bool matchesTest = true;
           if (listener.hasErrorTest) {
             _FutureErrorTest test = listener._errorTest;
             try {
               matchesTest = zone.runUnary(test, asyncError.error);
             } catch (e, s) {
-              listenerValueOrError = identical(asyncError.error, e) ?
-                  asyncError : new AsyncError(e, s);
-              listenerHasValue = false;
+              listenerValueOrError = identical(asyncError.error, e)
+                  ? asyncError
+                  : new AsyncError(e, s);
+              listenerHasError = true;
               return;
             }
           }
           Function errorCallback = listener._onError;
           if (matchesTest && errorCallback != null) {
             try {
               if (errorCallback is ZoneBinaryCallback) {
                 listenerValueOrError = zone.runBinary(errorCallback,
                                                       asyncError.error,
                                                       asyncError.stackTrace);
               } else {
                 listenerValueOrError = zone.runUnary(errorCallback,
                                                      asyncError.error);
               }
+              listenerHasError = false;
             } catch (e, s) {
-              listenerValueOrError = identical(asyncError.error, e) ?
-                  asyncError : new AsyncError(e, s);
-              listenerHasValue = false;
+              listenerValueOrError = identical(asyncError.error, e)
+                  ? asyncError
+                  : new AsyncError(e, s);
+              listenerHasError = true;
               return;

[floitsch, 2015/12/09 22:52:02]

no need for the return here.

[Lasse Reichstein Nielsen, 2015/12/10 06:52:27]

Done.

             }
-            listenerHasValue = true;
-          } else {
-            // Copy over the error from the source.
-            listenerValueOrError = asyncError;
-            listenerHasValue = false;
           }
         }
 
         void handleWhenCompleteCallback() {
+          assert(!listener.handlesError);
           var completeResult;
           try {
             completeResult = zone.run(listener._whenCompleteAction);
           } catch (e, s) {
             if (hasError && identical(source._error.error, e)) {
               listenerValueOrError = source._error;
             } else {
               listenerValueOrError = new AsyncError(e, s);
             }
-            listenerHasValue = false;
+            listenerHasError = true;
             return;
           }
           if (completeResult is Future) {
-            _Future result = listener.result;
-            result._isChained = true;
-            isPropagationAborted = true;
-            completeResult.then((ignored) {
-              _propagateToListeners(source, new _FutureListener.chain(result));
-            }, 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);
+            if (completeResult is _Future && completeResult._isComplete) {
+              if (completeResult._hasError) {
+                listenerValueOrError = completeResult._error;
+                listenerHasError = true;
               }
-              _propagateToListeners(completeResult,
-                                    new _FutureListener.chain(result));
-            });
+              // Otherwise use the existing result of source.
+              return;
+            }
+            // We have to wait for the compelteResult future to complete

[floitsch, 2015/12/09 22:52:02]

completeResult

[Lasse Reichstein Nielsen, 2015/12/10 06:52:26]

Good catch. Done.

[floitsch, 2015/12/10 07:24:41]

Your bar is low for "good catches" ;)

+            // before knowing if it's an error or we should use the result
+            // of source.
+            var originalSource = source;
+            listenerValueOrError = completeResult.then((_) => originalSource);
+            listenerHasError = false;
           }
         }
 
-        if (!hasError) {
+        if (listener.handlesComplete) {
+          // The whenComplete-handler is not combined with normal value/error
+          // handling. This means at most one handleX method is called per
+          // listener.
+          assert(!listener.handlesValue);
+          assert(!listener.handlesError);
+          handleWhenCompleteCallback();
+        } else if (!hasError) {
           if (listener.handlesValue) {
-            listenerHasValue = handleValueCallback();
+            handleValueCallback();
           }
         } else {
-          handleError();
+          if (listener.handlesError) {
+            handleError();
+          }
         }
-        if (listener.handlesComplete) {
-          handleWhenCompleteCallback();
-        }
+
         // If we changed zone, oldZone will not be null.
         if (oldZone != null) Zone._leave(oldZone);
 
-        if (isPropagationAborted) return;
         // If the listener's value is a future we need to chain it. Note that
         // this can only happen if there is a callback. Since 'is' checks
         // can be expensive, we're trying to avoid it.

[floitsch, 2015/12/09 22:52:02]

Not sure if is-checks really are that expensive.
is-tests on Future are everywhere, so might as well just do an is-check here.

[Lasse Reichstein Nielsen, 2015/12/10 06:52:26]

Done.

-        if (listenerHasValue &&
-            !identical(sourceValue, listenerValueOrError) &&
+        if (!listenerHasError &&
+            !identical(sourceResult, listenerValueOrError) &&
             listenerValueOrError is Future) {
           Future chainSource = listenerValueOrError;
           // Shortcut if the chain-source is already completed. Just continue
           // the loop.
           _Future result = listener.result;
           if (chainSource is _Future) {
             if (chainSource._isComplete) {
               // propagate the value (simulating a tail call).
-              result._isChained = true;
+              result._setChained();
               source = chainSource;
               listeners = new _FutureListener.chain(result);
               continue;
             } else {
               _chainCoreFuture(chainSource, result);
             }
           } else {
             _chainForeignFuture(chainSource, result);
           }
           return;
         }
       }
       _Future result = listener.result;
       listeners = result._removeListeners();
-      if (listenerHasValue) {
+      if (!listenerHasError) {
         result._setValue(listenerValueOrError);
       } else {
         AsyncError asyncError = listenerValueOrError;
         result._setErrorObject(asyncError);
       }
       // Prepare for next round.
       source = result;
     }
   }
 
@@ skipping 24 matching lines @@
       }
     }, onError: (e, s) {
       if (timer.isActive) {
         timer.cancel();
         result._completeError(e, s);
       }
     });
     return result;
   }
 }