Make nullFuture be per-zone.

We introduced a `nullFuture` during the null-safety migration where
we changed some methods to no longer allow returning `null`,
and they therefore had to return a `Future`.
That affected timing, because returning `null` was processed
synchronously, and that change in timing made some tests fail.
Rather that fix the fragile tests, we made the function return
a recognizable future, a canonical `Future<Null>.value(null)`,
and then recognized it and took a synchronous path for it.

That caused other issues, because the future was created in the
root zone. (Well, originally, it was created in the first zone
which needed one, that was worse. Now it's created in the root zone.)
Some code tries to contain asynchrony inside a custom zone, and
then the get a `nullFuture` and calls `then` on it, and that
schedules a microtask in the root zone.
(It should probably have used the listener's zone, and not store
a zone in the future at all, but that's how it was first done,
and now people rely on that behavior too.)

This change creates a `null` future *per zone* (lazily initialized
when asked for). That should be sufficient because the code recognizing
a returned `null` future is generally running in the same zone,
but if any other code gets the `nullFuture`, it will be in the
expected zone for where it was requested.

This is a reland of commit a247b15

Change-Id: Ia113756de1f6d50af4b1abfec219d6b4dcd5d59b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249488
Reviewed-by: Nate Bosch <[email protected]>
Commit-Queue: Lasse Nielsen <[email protected]>

Author: lrhn

pkg/front_end/testcases/nnbd/issue42546.dart.weak.outline.expect

@@ -28,19 +28,19 @@ static method main() → dynamic
 
 
 Extra constant evaluation status:
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:821:13 -> SymbolConstant(#catchError)
-Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:821:13 -> ListConstant(const <Type*>[])
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:821:13 -> SymbolConstant(#test)
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:872:13 -> SymbolConstant(#whenComplete)
-Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:872:13 -> ListConstant(const <Type*>[])
-Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:872:13 -> MapConstant(const <Symbol*, dynamic>{})
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:916:13 -> SymbolConstant(#timeout)
-Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:916:13 -> ListConstant(const <Type*>[])
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:916:13 -> SymbolConstant(#onTimeout)
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:770:13 -> SymbolConstant(#then)
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:770:13 -> SymbolConstant(#onError)
-Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:881:13 -> SymbolConstant(#asStream)
-Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:881:13 -> ListConstant(const <Type*>[])
-Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:881:13 -> ListConstant(const <dynamic>[])
-Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:881:13 -> MapConstant(const <Symbol*, dynamic>{})
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:814:13 -> SymbolConstant(#catchError)
+Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:814:13 -> ListConstant(const <Type*>[])
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:814:13 -> SymbolConstant(#test)
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:865:13 -> SymbolConstant(#whenComplete)
+Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:865:13 -> ListConstant(const <Type*>[])
+Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:865:13 -> MapConstant(const <Symbol*, dynamic>{})
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:909:13 -> SymbolConstant(#timeout)
+Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:909:13 -> ListConstant(const <Type*>[])
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:909:13 -> SymbolConstant(#onTimeout)
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:763:13 -> SymbolConstant(#then)
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:763:13 -> SymbolConstant(#onError)
+Evaluated: SymbolLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:874:13 -> SymbolConstant(#asStream)
+Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:874:13 -> ListConstant(const <Type*>[])
+Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:874:13 -> ListConstant(const <dynamic>[])
+Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:874:13 -> MapConstant(const <Symbol*, dynamic>{})
 Extra constant evaluation: evaluated: 61, effectively constant: 15

sdk/lib/async/async.dart

@@ -110,7 +110,6 @@ import "dart:_internal"
         checkNotNullable,
         EmptyIterator,
         IterableElementError,
-        nullFuture,
         printToZone,
         printToConsole,
         Since,

sdk/lib/async/future.dart

@@ -223,13 +223,6 @@ abstract class FutureOr<T> {
 /// it's very clearly documented.
 @pragma("wasm:entry-point")
 abstract class Future<T> {
-  /// A `Future<Null>` completed with `null`.
-  ///
-  /// Currently shared with `dart:internal`.
-  /// If that future can be removed, then change this back to
-  /// `_Future<Null>.zoneValue(null, _rootZone);`
-  static final _Future<Null> _nullFuture = nullFuture as _Future<Null>;
-
   /// A `Future<bool>` completed with `false`.
   static final _Future<bool> _falseFuture =
       new _Future<bool>.zoneValue(false, _rootZone);

sdk/lib/async/stream.dart

@@ -515,7 +515,7 @@ abstract class Stream<T> {
       controller
         ..onCancel = () {
           timer.cancel();
-          return Future._nullFuture;
+          return Zone._current._nullFuture;
         }
         ..onPause = () {
           watch.stop();

sdk/lib/async/stream_controller.dart

@@ -588,7 +588,10 @@ abstract class _StreamController<T> implements _StreamControllerBase<T> {
   Future<void> get done => _ensureDoneFuture();
 
   Future<void> _ensureDoneFuture() =>
-      _doneFuture ??= _isCanceled ? Future._nullFuture : _Future<void>();
+      _doneFuture ??
+      (_isCanceled
+          ? Zone._current._nullFuture as Future<void>
+          : _doneFuture = _Future<void>());
 
   /// Send or enqueue a data event.
   void add(T value) {
@@ -919,7 +922,7 @@ class _AddStreamState<T> {
     var cancel = addSubscription.cancel();
     if (cancel == null) {
       addStreamFuture._asyncComplete(null);
-      return Future._nullFuture;
+      return Zone._current._nullFuture;
     }
     return cancel.whenComplete(() {
       addStreamFuture._asyncComplete(null);

sdk/lib/async/stream_impl.dart

@@ -197,7 +197,7 @@ class _BufferingStreamSubscription<T>
     if (!_isCanceled) {
       _cancel();
     }
-    return _cancelFuture ?? Future._nullFuture;
+    return _cancelFuture ?? Zone._current._nullFuture;
   }
 
   Future<E> asFuture<E>([E? futureValue]) {
@@ -217,7 +217,7 @@ class _BufferingStreamSubscription<T>
     };
     _onError = (Object error, StackTrace stackTrace) {
       Future cancelFuture = cancel();
-      if (!identical(cancelFuture, Future._nullFuture)) {
+      if (!identical(Zone._current._nullFuture, cancelFuture)) {
         cancelFuture.whenComplete(() {
           result._completeError(error, stackTrace);
         });
@@ -297,7 +297,7 @@ class _BufferingStreamSubscription<T>
   // Hooks called when the input is paused, unpaused or canceled.
   // These must not throw. If overwritten to call user code, include suitable
   // try/catch wrapping and send any errors to
-  // [_Zone.current.handleUncaughtError].
+  // [Zone._current.handleUncaughtError].
   void _onPause() {
     assert(_isInputPaused);
   }
@@ -352,7 +352,6 @@ class _BufferingStreamSubscription<T>
       // future to finish we must not report the error.
       if (_isCanceled && !_waitsForCancel) return;
       _state |= _STATE_IN_CALLBACK;
-      // TODO(floitsch): this dynamic should be 'void'.
       var onError = _onError;
       if (onError is void Function(Object, StackTrace)) {
         _zone.runBinaryGuarded<Object, StackTrace>(onError, error, stackTrace);
@@ -367,7 +366,7 @@ class _BufferingStreamSubscription<T>
       _cancel();
       var cancelFuture = _cancelFuture;
       if (cancelFuture != null &&
-          !identical(cancelFuture, Future._nullFuture)) {
+          !identical(Zone._current._nullFuture, cancelFuture)) {
         cancelFuture.whenComplete(sendError);
       } else {
         sendError();
@@ -396,7 +395,8 @@ class _BufferingStreamSubscription<T>
     _cancel();
     _state |= _STATE_WAIT_FOR_CANCEL;
     var cancelFuture = _cancelFuture;
-    if (cancelFuture != null && !identical(cancelFuture, Future._nullFuture)) {
+    if (cancelFuture != null &&
+        !identical(Zone._current._nullFuture, cancelFuture)) {
       cancelFuture.whenComplete(sendDone);
     } else {
       sendDone();
@@ -672,7 +672,7 @@ class _DoneStreamSubscription<T> implements StreamSubscription<T> {
     }
   }
 
-  Future cancel() => Future._nullFuture;
+  Future cancel() => Zone._current._nullFuture;
 
   Future<E> asFuture<E>([E? futureValue]) {
     E resultValue;
@@ -819,7 +819,7 @@ class _BroadcastSubscriptionWrapper<T> implements StreamSubscription<T> {
 
   Future cancel() {
     _stream._cancelSubscription();
-    return Future._nullFuture;
+    return Zone._current._nullFuture;
   }
 
   bool get isPaused {
@@ -963,7 +963,7 @@ class _StreamIterator<T> implements StreamIterator<T> {
       }
       return subscription.cancel();
     }
-    return Future._nullFuture;
+    return Zone._current._nullFuture;
   }
 
   void _onData(T data) {

sdk/lib/async/stream_pipe.dart

@@ -26,7 +26,8 @@ _runUserCode<T>(T userCode(), onSuccess(T value),
 void _cancelAndError(StreamSubscription subscription, _Future future,
     Object error, StackTrace stackTrace) {
   var cancelFuture = subscription.cancel();
-  if (cancelFuture != null && !identical(cancelFuture, Future._nullFuture)) {
+  if (cancelFuture != null &&
+      !identical(Zone._current._nullFuture, cancelFuture)) {
     cancelFuture.whenComplete(() => future._completeError(error, stackTrace));
   } else {
     future._completeError(error, stackTrace);
@@ -55,7 +56,8 @@ void Function(Object error, StackTrace stackTrace) _cancelAndErrorClosure(
   before completing with a value. */
 void _cancelAndValue(StreamSubscription subscription, _Future future, value) {
   var cancelFuture = subscription.cancel();
-  if (cancelFuture != null && !identical(cancelFuture, Future._nullFuture)) {
+  if (cancelFuture != null &&
+      !identical(Zone._current._nullFuture, cancelFuture)) {
     cancelFuture.whenComplete(() => future._complete(value));
   } else {
     future._complete(value);

sdk/lib/async/zone.dart

@@ -1078,6 +1078,28 @@ abstract class _Zone implements Zone {
           implZone, e, identical(error, e) ? stackTrace : s);
     }
   }
+
+  /// A reusable `null`-valued future per zone used by `dart:async`.
+  ///
+  /// **DO NOT USE.**
+  ///
+  /// This future is used in situations where a future is expected,
+  /// but no asynchronous computation actually happens,
+  /// like cancelling a stream from a controller with no `onCancel` callback.
+  /// *Some code depends on recognizing this future in order to react
+  /// synchronously.*
+  /// It does so to avoid changing event interleaving during the null safety
+  /// migration where, for example, the [StreamSubscription.cancel] method
+  /// stopped being able to return `null`.
+  /// The code that would be broken by such a timing change is fragile,
+  /// but we are not able to simply change it.
+  /// For better or worse, code depends on the precise timing
+  /// that our libraries have so far exhibited.
+  ///
+  /// This future will be removed again if we can ever do so.
+  /// Do not use it for anything other than preserving timing
+  /// during the null safety migration.
+  _Future<Null> get _nullFuture;
 }
 
 class _CustomZone extends _Zone {
@@ -1105,6 +1127,9 @@ class _CustomZone extends _Zone {
   /// The parent zone.
   final _Zone parent;
 
+  /// Cached value for [_nullFuture];
+  _Future<Null>? _nullFutureCache;
+
   /// The zone's scoped value declaration map.
   ///
   /// This is always a [HashMap].
@@ -1196,6 +1221,18 @@ class _CustomZone extends _Zone {
   /// parent's error-zone.
   Zone get errorZone => _handleUncaughtError.zone;
 
+  _Future<Null> get _nullFuture {
+    _Future<Null>? result = _nullFutureCache;
+    if (result != null) return result;
+    // We only care about the zone of the null future
+    // because of the zone it schedules microtasks in.
+    var microtaskZone = _scheduleMicrotask.zone;
+    if (!identical(microtaskZone, this)) {
+      return _nullFutureCache = microtaskZone._nullFuture;
+    }
+    return _nullFutureCache = _Future<Null>.value(null);
+  }
+
   void runGuarded(void f()) {
     try {
       run(f);
@@ -1509,6 +1546,8 @@ Zone _rootFork(Zone? self, ZoneDelegate? parent, Zone zone,
 }
 
 class _RootZone extends _Zone {
+  static final _nullFutureCache = _Future<Null>.zoneValue(null, _rootZone);
+
   const _RootZone();
 
   _ZoneFunction<RunHandler> get _run =>
@@ -1547,6 +1586,8 @@ class _RootZone extends _Zone {
   // The parent zone.
   _Zone? get parent => null;
 
+  _Future<Null> get _nullFuture => _nullFutureCache;
+
   /// The zone's scoped value declaration map.
   ///
   /// This is always a [HashMap].

sdk/lib/html/dart2js/html_dart2js.dart

@@ -37302,13 +37302,13 @@ class _EventStreamSubscription<T extends Event> extends StreamSubscription<T> {
   }
 
   Future cancel() {
-    if (_canceled) return nullFuture;
-
-    _unlisten();
-    // Clear out the target to indicate this is complete.
-    _target = null;
-    _onData = null;
-    return nullFuture;
+    if (!_canceled) {
+      _unlisten();
+      // Clear out the target to indicate this is complete.
+      _target = null;
+      _onData = null;
+    }
+    return Future<void>.value(null);
   }
 
   bool get _canceled => _target == null;

sdk/lib/internal/internal.dart

@@ -136,27 +136,7 @@ int parseHexByte(String source, int index) {
   return digit1 * 16 + digit2 - (digit2 & 256);
 }
 
-/// A reusable `null`-valued future used by `dart:async`.
-///
-/// **DO NOT USE.**
-///
-/// This future is used in situations where a future is expected,
-/// but no asynchronous computation actually happens,
-/// like cancelling a stream from a controller with no `onCancel` callback.
-/// *Some code depends on recognizing this future in order to react
-/// synchronously.*
-/// It does so to avoid changing event interleaving during the null safety
-/// migration where, for example, the [StreamSubscription.cancel] method
-/// stopped being able to return `null`.
-/// The code that would be broken by such a timing change is fragile,
-/// but we are not able to simply change it.
-/// For better or worse, code depends on the precise timing that our libraries
-/// have so far exhibited.
-///
-/// This future will be removed again if we can ever do so.
-/// Do not use it for anything other than preserving timing
-/// during the null safety migration.
-final Future<Null> nullFuture = Zone.root.run(() => Future<Null>.value(null));
+final Expando<Future<Null>> _nullFutures = Expando<Future<Null>>();
 
 /// A default hash function used by the platform in various places.
 ///

tests/lib/async/null_future_zone_test.dart

@@ -13,21 +13,32 @@ main() {
     Expect.isFalse(await it.moveNext());
 
     late Future nullFuture;
-    late Future falseFuture;
 
+    bool nullFutureZoneUsed = false;
     runZoned(() {
       nullFuture = (new StreamController()..stream.listen(null).cancel()).done;
-      falseFuture = it.moveNext();
     }, zoneSpecification: new ZoneSpecification(scheduleMicrotask:
         (Zone self, ZoneDelegate parent, Zone zone, void f()) {
-      Expect.fail("Should not be called");
+      Expect.identical(zone, self);
+      nullFutureZoneUsed = true;
+      parent.scheduleMicrotask(zone, f);
     }));
 
     nullFuture.then((value) {
       Expect.isNull(value);
+      Expect.isTrue(nullFutureZoneUsed);
       asyncEnd();
     });
 
+    late Future falseFuture;
+
+    runZoned(() {
+      falseFuture = it.moveNext();
+    }, zoneSpecification: new ZoneSpecification(scheduleMicrotask:
+        (Zone self, ZoneDelegate parent, Zone zone, void f()) {
+      Expect.fail("Should not be called");
+    }));
+
     falseFuture.then((value) {
       Expect.isFalse(value);
       asyncEnd();

tests/lib_2/async/null_future_zone_test.dart

@@ -15,21 +15,32 @@ main() {
     Expect.isFalse(await it.moveNext());
 
     Future nullFuture;
-    Future falseFuture;
 
+    bool nullFutureZoneUsed = false;
     runZoned(() {
       nullFuture = (new StreamController()..stream.listen(null).cancel()).done;
-      falseFuture = it.moveNext();
     }, zoneSpecification: new ZoneSpecification(scheduleMicrotask:
         (Zone self, ZoneDelegate parent, Zone zone, void f()) {
-      Expect.fail("Should not be called");
+      Expect.identical(zone, self);
+      nullFutureZoneUsed = true;
+      parent.scheduleMicrotask(zone, f);
     }));
 
     nullFuture.then((value) {
       Expect.isNull(value);
+      Expect.isTrue(nullFutureZoneUsed);
       asyncEnd();
     });
 
+    Future falseFuture;
+
+    runZoned(() {
+      falseFuture = it.moveNext();
+    }, zoneSpecification: new ZoneSpecification(scheduleMicrotask:
+        (Zone self, ZoneDelegate parent, Zone zone, void f()) {
+      Expect.fail("Should not be called");
+    }));
+
     falseFuture.then((value) {
       Expect.isFalse(value);
       asyncEnd();