From d9d7d3082029480da3f8185b79ce7934efd1f650 Mon Sep 17 00:00:00 2001 From: Emmanuel Pellereau Date: Fri, 24 Jun 2022 06:48:49 +0000 Subject: [PATCH] Revert "Make `nullFuture` be per-zone." This reverts commit 4d750a862d3f659107e0101d3035bcdc2ca3b2ee. Reason for revert: breaks google3 (b/236665701) Original change's description: > 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 TBR=lrn@google.com,nbosch@google.com Change-Id: I02d62d58bae33d6a606a80eb3eee2e8e721a8e20 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249620 Commit-Queue: Emmanuel Pellereau Reviewed-by: Emmanuel Pellereau Reviewed-by: Nate Bosch --- .../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 | 17 ++------ tests/lib_2/async/null_future_zone_test.dart | 17 ++------ tools/dom/src/EventStreamProvider.dart | 14 +++---- 13 files changed, 78 insertions(+), 118 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 ec6a7f6922c0..e2189027e1f6 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: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 {}) +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 {}) Extra constant evaluation: evaluated: 61, effectively constant: 15 diff --git a/sdk/lib/async/async.dart b/sdk/lib/async/async.dart index bd41381cca82..ca05fc66a653 100644 --- a/sdk/lib/async/async.dart +++ b/sdk/lib/async/async.dart @@ -110,6 +110,7 @@ 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 908e5e198c09..f83386d30a32 100644 --- a/sdk/lib/async/future.dart +++ b/sdk/lib/async/future.dart @@ -223,6 +223,13 @@ 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 117465c4405c..038c3bb5513c 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 Zone._current._nullFuture; + return Future._nullFuture; } ..onPause = () { watch.stop(); diff --git a/sdk/lib/async/stream_controller.dart b/sdk/lib/async/stream_controller.dart index 1f34d766543c..2baf00836a8a 100644 --- a/sdk/lib/async/stream_controller.dart +++ b/sdk/lib/async/stream_controller.dart @@ -588,10 +588,7 @@ abstract class _StreamController implements _StreamControllerBase { Future get done => _ensureDoneFuture(); Future _ensureDoneFuture() => - _doneFuture ?? - (_isCanceled - ? Zone._current._nullFuture as Future - : _doneFuture = _Future()); + _doneFuture ??= _isCanceled ? Future._nullFuture : _Future(); /// Send or enqueue a data event. void add(T value) { @@ -922,7 +919,7 @@ class _AddStreamState { var cancel = addSubscription.cancel(); if (cancel == null) { addStreamFuture._asyncComplete(null); - return Zone._current._nullFuture; + return Future._nullFuture; } return cancel.whenComplete(() { addStreamFuture._asyncComplete(null); diff --git a/sdk/lib/async/stream_impl.dart b/sdk/lib/async/stream_impl.dart index b5a40cbddc0a..b4fb5d8df185 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 ?? Zone._current._nullFuture; + return _cancelFuture ?? Future._nullFuture; } Future asFuture([E? futureValue]) { @@ -217,7 +217,7 @@ class _BufferingStreamSubscription }; _onError = (Object error, StackTrace stackTrace) { Future cancelFuture = cancel(); - if (!identical(Zone._current._nullFuture, cancelFuture)) { + if (!identical(cancelFuture, Future._nullFuture)) { 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,6 +352,7 @@ 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); @@ -366,7 +367,7 @@ class _BufferingStreamSubscription _cancel(); var cancelFuture = _cancelFuture; if (cancelFuture != null && - !identical(Zone._current._nullFuture, cancelFuture)) { + !identical(cancelFuture, Future._nullFuture)) { cancelFuture.whenComplete(sendError); } else { sendError(); @@ -395,8 +396,7 @@ class _BufferingStreamSubscription _cancel(); _state |= _STATE_WAIT_FOR_CANCEL; var cancelFuture = _cancelFuture; - if (cancelFuture != null && - !identical(Zone._current._nullFuture, cancelFuture)) { + if (cancelFuture != null && !identical(cancelFuture, Future._nullFuture)) { cancelFuture.whenComplete(sendDone); } else { sendDone(); @@ -672,7 +672,7 @@ class _DoneStreamSubscription implements StreamSubscription { } } - Future cancel() => Zone._current._nullFuture; + Future cancel() => Future._nullFuture; Future asFuture([E? futureValue]) { E resultValue; @@ -819,7 +819,7 @@ class _BroadcastSubscriptionWrapper implements StreamSubscription { Future cancel() { _stream._cancelSubscription(); - return Zone._current._nullFuture; + return Future._nullFuture; } bool get isPaused { @@ -963,7 +963,7 @@ class _StreamIterator implements StreamIterator { } return subscription.cancel(); } - return Zone._current._nullFuture; + return Future._nullFuture; } void _onData(T data) { diff --git a/sdk/lib/async/stream_pipe.dart b/sdk/lib/async/stream_pipe.dart index 2caa84b5c7b5..cd707be13e45 100644 --- a/sdk/lib/async/stream_pipe.dart +++ b/sdk/lib/async/stream_pipe.dart @@ -26,8 +26,7 @@ _runUserCode(T userCode(), onSuccess(T value), void _cancelAndError(StreamSubscription subscription, _Future future, Object error, StackTrace stackTrace) { var cancelFuture = subscription.cancel(); - if (cancelFuture != null && - !identical(Zone._current._nullFuture, cancelFuture)) { + if (cancelFuture != null && !identical(cancelFuture, Future._nullFuture)) { cancelFuture.whenComplete(() => future._completeError(error, stackTrace)); } else { future._completeError(error, stackTrace); @@ -56,8 +55,7 @@ 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(Zone._current._nullFuture, cancelFuture)) { + if (cancelFuture != null && !identical(cancelFuture, Future._nullFuture)) { cancelFuture.whenComplete(() => future._complete(value)); } else { future._complete(value); diff --git a/sdk/lib/async/zone.dart b/sdk/lib/async/zone.dart index 42e0c76d90cf..b9bcd1991522 100644 --- a/sdk/lib/async/zone.dart +++ b/sdk/lib/async/zone.dart @@ -1078,28 +1078,6 @@ 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 { @@ -1127,9 +1105,6 @@ 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]. @@ -1221,18 +1196,6 @@ 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); @@ -1546,8 +1509,6 @@ Zone _rootFork(Zone? self, ZoneDelegate? parent, Zone zone, } class _RootZone extends _Zone { - static final _nullFutureCache = _Future.zoneValue(null, _rootZone); - const _RootZone(); _ZoneFunction get _run => @@ -1586,8 +1547,6 @@ 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 91224f6ee837..671f68a0a53b 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) { - _unlisten(); - // Clear out the target to indicate this is complete. - _target = null; - _onData = null; - } - return Future.value(null); + if (_canceled) return nullFuture; + + _unlisten(); + // Clear out the target to indicate this is complete. + _target = null; + _onData = null; + return nullFuture; } bool get _canceled => _target == null; diff --git a/sdk/lib/internal/internal.dart b/sdk/lib/internal/internal.dart index 0353c7195146..ff3d890b1880 100644 --- a/sdk/lib/internal/internal.dart +++ b/sdk/lib/internal/internal.dart @@ -136,7 +136,27 @@ int parseHexByte(String source, int index) { return digit1 * 16 + digit2 - (digit2 & 256); } -final Expando> _nullFutures = Expando>(); +/// 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)); /// 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 34201e726288..9a266bee8a14 100644 --- a/tests/lib/async/null_future_zone_test.dart +++ b/tests/lib/async/null_future_zone_test.dart @@ -13,32 +13,21 @@ 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.identical(zone, self); - nullFutureZoneUsed = true; - parent.scheduleMicrotask(zone, f); + Expect.fail("Should not be called"); })); 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(); diff --git a/tests/lib_2/async/null_future_zone_test.dart b/tests/lib_2/async/null_future_zone_test.dart index 77cb1b7fcbf7..332291603968 100644 --- a/tests/lib_2/async/null_future_zone_test.dart +++ b/tests/lib_2/async/null_future_zone_test.dart @@ -15,32 +15,21 @@ 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.identical(zone, self); - nullFutureZoneUsed = true; - parent.scheduleMicrotask(zone, f); + Expect.fail("Should not be called"); })); 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(); diff --git a/tools/dom/src/EventStreamProvider.dart b/tools/dom/src/EventStreamProvider.dart index 1ae62681c94e..90b19aaa4d54 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) { - _unlisten(); - // Clear out the target to indicate this is complete. - _target = null; - _onData = null; - } - return Future.value(null); + if (_canceled) return nullFuture; + + _unlisten(); + // Clear out the target to indicate this is complete. + _target = null; + _onData = null; + return nullFuture; } bool get _canceled => _target == null;