From 4d750a862d3f659107e0101d3035bcdc2ca3b2ee Mon Sep 17 00:00:00 2001 From: "Lasse R.H. Nielsen" Date: Thu, 23 Jun 2022 09:19:45 +0000 Subject: [PATCH] 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.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 a247b158d6fc2ae89e824080bd7877e7b51f642a Change-Id: Ia113756de1f6d50af4b1abfec219d6b4dcd5d59b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249488 Reviewed-by: Nate Bosch Commit-Queue: Lasse Nielsen --- .../nnbd/issue42546.dart.weak.outline.expect | 30 +++++++------- sdk/lib/async/async.dart | 1 - sdk/lib/async/future.dart | 7 ---- sdk/lib/async/stream.dart | 2 +- sdk/lib/async/stream_controller.dart | 7 +++- sdk/lib/async/stream_impl.dart | 18 ++++---- sdk/lib/async/stream_pipe.dart | 6 ++- sdk/lib/async/zone.dart | 41 +++++++++++++++++++ sdk/lib/html/dart2js/html_dart2js.dart | 14 +++---- sdk/lib/internal/internal.dart | 22 +--------- tests/lib/async/null_future_zone_test.dart | 23 ++++++++--- tests/lib_2/async/null_future_zone_test.dart | 23 ++++++++--- tools/dom/src/EventStreamProvider.dart | 14 +++---- 13 files changed, 124 insertions(+), 84 deletions(-) diff --git a/pkg/front_end/testcases/nnbd/issue42546.dart.weak.outline.expect b/pkg/front_end/testcases/nnbd/issue42546.dart.weak.outline.expect index e2189027e1f..ec6a7f6922c 100644 --- a/pkg/front_end/testcases/nnbd/issue42546.dart.weak.outline.expect +++ b/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 []) -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 []) -Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:872:13 -> MapConstant(const {}) -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 []) -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 []) -Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:881:13 -> ListConstant(const []) -Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:881:13 -> MapConstant(const {}) +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 []) +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 []) +Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:865:13 -> MapConstant(const {}) +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 []) +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 []) +Evaluated: ListLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:874:13 -> ListConstant(const []) +Evaluated: MapLiteral @ org-dartlang-sdk:///sdk/lib/async/future.dart:874:13 -> MapConstant(const {}) Extra constant evaluation: evaluated: 61, effectively constant: 15 diff --git a/sdk/lib/async/async.dart b/sdk/lib/async/async.dart index ca05fc66a65..bd41381cca8 100644 --- a/sdk/lib/async/async.dart +++ b/sdk/lib/async/async.dart @@ -110,7 +110,6 @@ import "dart:_internal" checkNotNullable, EmptyIterator, IterableElementError, - nullFuture, printToZone, printToConsole, Since, diff --git a/sdk/lib/async/future.dart b/sdk/lib/async/future.dart index f83386d30a3..908e5e198c0 100644 --- a/sdk/lib/async/future.dart +++ b/sdk/lib/async/future.dart @@ -223,13 +223,6 @@ abstract class FutureOr { /// it's very clearly documented. @pragma("wasm:entry-point") abstract class Future { - /// A `Future` completed with `null`. - /// - /// Currently shared with `dart:internal`. - /// If that future can be removed, then change this back to - /// `_Future.zoneValue(null, _rootZone);` - static final _Future _nullFuture = nullFuture as _Future; - /// A `Future` completed with `false`. static final _Future _falseFuture = new _Future.zoneValue(false, _rootZone); diff --git a/sdk/lib/async/stream.dart b/sdk/lib/async/stream.dart index 038c3bb5513..117465c4405 100644 --- a/sdk/lib/async/stream.dart +++ b/sdk/lib/async/stream.dart @@ -515,7 +515,7 @@ abstract class Stream { controller ..onCancel = () { timer.cancel(); - return Future._nullFuture; + return Zone._current._nullFuture; } ..onPause = () { watch.stop(); diff --git a/sdk/lib/async/stream_controller.dart b/sdk/lib/async/stream_controller.dart index 2baf00836a8..1f34d766543 100644 --- a/sdk/lib/async/stream_controller.dart +++ b/sdk/lib/async/stream_controller.dart @@ -588,7 +588,10 @@ abstract class _StreamController implements _StreamControllerBase { Future get done => _ensureDoneFuture(); Future _ensureDoneFuture() => - _doneFuture ??= _isCanceled ? Future._nullFuture : _Future(); + _doneFuture ?? + (_isCanceled + ? Zone._current._nullFuture as Future + : _doneFuture = _Future()); /// Send or enqueue a data event. void add(T value) { @@ -919,7 +922,7 @@ class _AddStreamState { var cancel = addSubscription.cancel(); if (cancel == null) { addStreamFuture._asyncComplete(null); - return Future._nullFuture; + return Zone._current._nullFuture; } return cancel.whenComplete(() { addStreamFuture._asyncComplete(null); diff --git a/sdk/lib/async/stream_impl.dart b/sdk/lib/async/stream_impl.dart index b4fb5d8df18..b5a40cbddc0 100644 --- a/sdk/lib/async/stream_impl.dart +++ b/sdk/lib/async/stream_impl.dart @@ -197,7 +197,7 @@ class _BufferingStreamSubscription if (!_isCanceled) { _cancel(); } - return _cancelFuture ?? Future._nullFuture; + return _cancelFuture ?? Zone._current._nullFuture; } Future asFuture([E? futureValue]) { @@ -217,7 +217,7 @@ class _BufferingStreamSubscription }; _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 // 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 // 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(onError, error, stackTrace); @@ -367,7 +366,7 @@ class _BufferingStreamSubscription _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 _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 implements StreamSubscription { } } - Future cancel() => Future._nullFuture; + Future cancel() => Zone._current._nullFuture; Future asFuture([E? futureValue]) { E resultValue; @@ -819,7 +819,7 @@ class _BroadcastSubscriptionWrapper implements StreamSubscription { Future cancel() { _stream._cancelSubscription(); - return Future._nullFuture; + return Zone._current._nullFuture; } bool get isPaused { @@ -963,7 +963,7 @@ class _StreamIterator implements StreamIterator { } return subscription.cancel(); } - return Future._nullFuture; + return Zone._current._nullFuture; } void _onData(T data) { diff --git a/sdk/lib/async/stream_pipe.dart b/sdk/lib/async/stream_pipe.dart index cd707be13e4..2caa84b5c7b 100644 --- a/sdk/lib/async/stream_pipe.dart +++ b/sdk/lib/async/stream_pipe.dart @@ -26,7 +26,8 @@ _runUserCode(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); diff --git a/sdk/lib/async/zone.dart b/sdk/lib/async/zone.dart index b9bcd199152..42e0c76d90c 100644 --- a/sdk/lib/async/zone.dart +++ b/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 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? _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 get _nullFuture { + _Future? 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.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.zoneValue(null, _rootZone); + const _RootZone(); _ZoneFunction get _run => @@ -1547,6 +1586,8 @@ class _RootZone extends _Zone { // The parent zone. _Zone? get parent => null; + _Future get _nullFuture => _nullFutureCache; + /// The zone's scoped value declaration map. /// /// This is always a [HashMap]. diff --git a/sdk/lib/html/dart2js/html_dart2js.dart b/sdk/lib/html/dart2js/html_dart2js.dart index 671f68a0a53..91224f6ee83 100644 --- a/sdk/lib/html/dart2js/html_dart2js.dart +++ b/sdk/lib/html/dart2js/html_dart2js.dart @@ -37302,13 +37302,13 @@ class _EventStreamSubscription extends StreamSubscription { } 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.value(null); } bool get _canceled => _target == null; diff --git a/sdk/lib/internal/internal.dart b/sdk/lib/internal/internal.dart index ff3d890b188..0353c719514 100644 --- a/sdk/lib/internal/internal.dart +++ b/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 nullFuture = Zone.root.run(() => Future.value(null)); +final Expando> _nullFutures = Expando>(); /// A default hash function used by the platform in various places. /// diff --git a/tests/lib/async/null_future_zone_test.dart b/tests/lib/async/null_future_zone_test.dart index 9a266bee8a1..34201e72628 100644 --- a/tests/lib/async/null_future_zone_test.dart +++ b/tests/lib/async/null_future_zone_test.dart @@ -13,21 +13,32 @@ main() { Expect.isFalse(await it.moveNext()); late Future nullFuture; + + bool nullFutureZoneUsed = false; + runZoned(() { + nullFuture = (new StreamController()..stream.listen(null).cancel()).done; + }, zoneSpecification: new ZoneSpecification(scheduleMicrotask: + (Zone self, ZoneDelegate parent, Zone zone, void f()) { + Expect.identical(zone, self); + nullFutureZoneUsed = true; + parent.scheduleMicrotask(zone, f); + })); + + nullFuture.then((value) { + Expect.isNull(value); + Expect.isTrue(nullFutureZoneUsed); + asyncEnd(); + }); + late Future falseFuture; 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"); })); - nullFuture.then((value) { - Expect.isNull(value); - asyncEnd(); - }); - falseFuture.then((value) { Expect.isFalse(value); asyncEnd(); diff --git a/tests/lib_2/async/null_future_zone_test.dart b/tests/lib_2/async/null_future_zone_test.dart index 33229160396..77cb1b7fcbf 100644 --- a/tests/lib_2/async/null_future_zone_test.dart +++ b/tests/lib_2/async/null_future_zone_test.dart @@ -15,21 +15,32 @@ main() { Expect.isFalse(await it.moveNext()); Future nullFuture; + + bool nullFutureZoneUsed = false; + runZoned(() { + nullFuture = (new StreamController()..stream.listen(null).cancel()).done; + }, zoneSpecification: new ZoneSpecification(scheduleMicrotask: + (Zone self, ZoneDelegate parent, Zone zone, void f()) { + Expect.identical(zone, self); + nullFutureZoneUsed = true; + parent.scheduleMicrotask(zone, f); + })); + + nullFuture.then((value) { + Expect.isNull(value); + Expect.isTrue(nullFutureZoneUsed); + asyncEnd(); + }); + Future falseFuture; 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"); })); - nullFuture.then((value) { - Expect.isNull(value); - asyncEnd(); - }); - falseFuture.then((value) { Expect.isFalse(value); asyncEnd(); diff --git a/tools/dom/src/EventStreamProvider.dart b/tools/dom/src/EventStreamProvider.dart index 90b19aaa4d5..1ae62681c94 100644 --- a/tools/dom/src/EventStreamProvider.dart +++ b/tools/dom/src/EventStreamProvider.dart @@ -248,13 +248,13 @@ class _EventStreamSubscription extends StreamSubscription { } 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.value(null); } bool get _canceled => _target == null;