Skip to content

Commit

Permalink
Fix issue 56806, incorrect ignore on futures.
Browse files Browse the repository at this point in the history
Makes `whenComplete` create a new future with either the result of the
original, or the error of a future returned by the callback.
Until now it was returning the original future, causing it to be chained after
it was completed. That caused issue with `ignore()`.
(Which suggests that there is still something iffy about ignore and chaining.)

Combines `chainCoreFuture{Sync,Async}` into one function with boolean flag.
Most of the code is the same, and they had accidentally drifted apart.

Fixes #56806.

CoreLibraryReviewExempt: Should be simple refactoring.
Tested: Regression test added
Bug: http://dartbug.com/56806
Change-Id: I52751c078a4e26a10e9da0a64460e2c5c7f34ae5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/387360
Commit-Queue: Lasse Nielsen <[email protected]>
Reviewed-by: Nate Bosch <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
  • Loading branch information
lrhn authored and Commit Queue committed Oct 15, 2024
1 parent caba043 commit 6f1b279
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class B<T extends core::Object? = dynamic> extends self::A<core::String> impleme

[@vm.inferred-return-type.metadata=!]
[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4]
no-such-method-forwarder method then<R extends core::Object? = dynamic>([@vm.inferred-arg-type.metadata=dart.core::_Closure] (self::B::T%) → FutureOr<self::B::then::R%>onValue, {[@vm.inferred-arg-type.metadata=dart.core::_Closure?] core::Function? onError = #C1}) → asy::Future<self::B::then::R%>
no-such-method-forwarder method then<R extends core::Object? = dynamic>([@vm.inferred-arg-type.metadata=dart.core::_Closure] (self::B::T%) → FutureOr<self::B::then::R%>onValue, {[@vm.inferred-arg-type.metadata=dart.core::_Closure] core::Function? onError = #C1}) → asy::Future<self::B::then::R%>
return _in::unsafeCast<asy::Future<self::B::then::R%>>([@vm.direct-call.metadata=#lib::B.noSuchMethod] [@vm.inferred-type.metadata=! (skip check)] this.{self::B::noSuchMethod}(new core::_InvocationMirror::_withType(#C2, 0, [@vm.inferred-type.metadata=dart.core::_ImmutableList] core::List::unmodifiable<core::Type>([@vm.inferred-type.metadata=dart.core::_GrowableList<dart.core::Type>] core::_GrowableList::_literal1<core::Type>(self::B::then::R%)), [@vm.inferred-type.metadata=dart.core::_ImmutableList] core::List::unmodifiable<dynamic>([@vm.inferred-type.metadata=dart.core::_GrowableList<dynamic>] core::_GrowableList::_literal1<dynamic>(onValue)), [@vm.inferred-type.metadata=dart.collection::UnmodifiableMapView<dart.core::Symbol, dynamic>] core::Map::unmodifiable<core::Symbol, dynamic>(<core::Symbol, dynamic>{#C3: onError}))){(core::Invocation) → dynamic});

[@vm.inferred-return-type.metadata=!]
Expand Down
8 changes: 2 additions & 6 deletions sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,7 @@ _async<T>(Function() initGenerator) {
// instead treat it as a completed value.
if (value is Future) {
if (value is _Future) {
if (isRunningAsEvent) {
_Future._chainCoreFutureSync(value, asyncFuture);
} else {
_Future._chainCoreFutureAsync(value, asyncFuture);
}
_Future._chainCoreFuture(value, asyncFuture, isRunningAsEvent);
} else {
asyncFuture._chainForeignFuture(value);
}
Expand All @@ -121,7 +117,7 @@ _async<T>(Function() initGenerator) {
asyncFuture._asyncComplete(JS('', '#', value));
}
} else {
_Future._chainCoreFutureSync(onAwait(value), asyncFuture);
_Future._chainCoreFuture(onAwait(value), asyncFuture, true);
}
} catch (e, s) {
if (isRunningAsEvent) {
Expand Down
104 changes: 58 additions & 46 deletions sdk/lib/async/future_impl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,9 @@ class _Future<T> implements Future<T> {
// This constructor is used by async/await.
_Future() : _zone = Zone._current;

// Empty `_Future` in a given zone.
_Future.zone(this._zone);

// Constructor used by [Future.value].
_Future.immediate(FutureOr<T> result) : _zone = Zone._current {
_asyncComplete(result);
Expand Down Expand Up @@ -397,11 +400,18 @@ class _Future<T> implements Future<T> {
}

void _ignore() {
_Future<Object?> source = this;
while (source._isChained) {
source = source._chainSource;
_state |= _stateIgnoreError;
if (_isChained) {
// Mark chain source. If it has no listeners at all, then neither
// does this future. (But this may suppress errors on other futures
// with the same chain source and no listeners, which haven't been
// ignored.)
_Future<Object?> source = this;
do {
source = source._chainSource;
} while (source._isChained);
source._state |= _stateIgnoreError;
}
source._state |= _stateIgnoreError;
}

Future<T> catchError(Function onError, {bool test(Object error)?}) {
Expand Down Expand Up @@ -573,7 +583,7 @@ class _Future<T> implements Future<T> {
/// Completes this future with the result of [source].
///
/// The [source] future should not be a [_Future], use
/// [_chainCoreFutureSync] for those.
/// [_chainCoreFuture] for those.
///
/// Since [source] is an unknown [Future], it's interacted with
/// through [Future.then], which is required to be asynchronous.
Expand Down Expand Up @@ -613,37 +623,7 @@ class _Future<T> implements Future<T> {
/// propagated to the target future's listeners.
/// If the source future is not completed, the target future is made
/// to listen for its completion.
static void _chainCoreFutureSync(_Future source, _Future target) {
assert(target._mayAddListener); // Not completed, not already chained.
while (source._isChained) {
source = source._chainSource;
}
if (identical(source, target)) {
target._asyncCompleteError(
ArgumentError.value(
source, null, "Cannot complete a future with itself"),
StackTrace.current);
return;
}
source._state |= target._state & _stateIgnoreError;
if (source._isComplete) {
_FutureListener? listeners = target._removeListeners();
target._cloneResult(source);
_propagateToListeners(target, listeners);
} else {
_FutureListener? listeners = target._resultOrListeners;
target._setChained(source);
source._prependListeners(listeners);
}
}

/// Asynchronously completes a [target] future with a [source] future.
///
/// If the [source] future is already completed, its result is
/// asynchronously propagated to the [target] future's listeners.
/// If the [source] future is not completed, the [target] future is made
/// to listen for its completion.
static void _chainCoreFutureAsync(_Future source, _Future target) {
static void _chainCoreFuture(_Future source, _Future target, bool sync) {
assert(target._mayAddListener); // Not completed, not already chained.
while (source._isChained) {
source = source._chainSource;
Expand All @@ -655,26 +635,32 @@ class _Future<T> implements Future<T> {
StackTrace.current);
return;
}
var ignoreError = target._state & _stateIgnoreError;
source._state |= ignoreError;
if (!source._isComplete) {
// Chain immediately if the source is not complete.
// This won't call any listeners.
// This won't call any listeners, whether completing sync or async.
_FutureListener? listeners = target._resultOrListeners;
target._setChained(source);
source._prependListeners(listeners);
return;
}

// Complete a value synchronously, if no-one is listening.
// This won't call any listeners.
if (!source._hasError && target._resultOrListeners == null) {
if (sync ||
(target._resultOrListeners == null &&
(!source._hasError || ignoreError != 0))) {
// Complete synchronously when allowed.
// If no-one is listening this won't call any listeners synchronously.
// Do delay un-ignored errors to give time to add listeners.
_FutureListener? listeners = target._removeListeners();
target._cloneResult(source);
_propagateToListeners(target, listeners);
return;
}

// Otherwise delay the chaining to avoid any synchronous callbacks.
target._setPendingComplete();
target._zone.scheduleMicrotask(() {
_chainCoreFutureSync(source, target);
_chainCoreFuture(source, target, _allowCompleteSync);
});
}

Expand All @@ -688,7 +674,7 @@ class _Future<T> implements Future<T> {
assert(!_isComplete);
if (value is Future<T>) {
if (value is _Future<T>) {
_chainCoreFutureSync(value, this);
_chainCoreFuture(value, this, _allowCompleteSync);
} else {
_chainForeignFuture(value);
}
Expand All @@ -707,6 +693,16 @@ class _Future<T> implements Future<T> {
_propagateToListeners(this, listeners);
}

void _completeWithResultOf(_Future<Object?> source) {
assert(source._isComplete);
if (source._hasError && !_zone.inSameErrorZone(source._zone)) {
return;
}
_FutureListener? listeners = _removeListeners();
_cloneResult(source);
_propagateToListeners(this, listeners);
}

void _completeError(Object error, StackTrace stackTrace) {
assert(!_isComplete);

Expand Down Expand Up @@ -786,7 +782,7 @@ class _Future<T> implements Future<T> {
assert(_mayComplete);
if (value is _Future<T>) {
// Chain ensuring that we don't complete synchronously.
_chainCoreFutureAsync(value, this);
_chainCoreFuture(value, this, _requireCompleteAsync);
return;
}
// Just listen on the foreign future. This guarantees an async delay.
Expand Down Expand Up @@ -892,7 +888,13 @@ class _Future<T> implements Future<T> {
// before knowing if it's an error or we should use the result
// of source.
var originalSource = source;
listenerValueOrError = completeResult.then((_) => originalSource);
var joinedResult = source._newFutureWithSameType();
completeResult.then<void>((_) {
joinedResult._completeWithResultOf(originalSource);
}, onError: (Object e, StackTrace s) {
joinedResult._completeError(e, s);
});
listenerValueOrError = joinedResult;
listenerHasError = false;
}
}
Expand Down Expand Up @@ -954,7 +956,7 @@ class _Future<T> implements Future<T> {
source = chainSource;
continue;
} else {
_chainCoreFutureSync(chainSource, result);
_chainCoreFuture(chainSource, result, _allowCompleteSync);
}
} else {
result._chainForeignFuture(chainSource);
Expand All @@ -976,6 +978,12 @@ class _Future<T> implements Future<T> {
}
}

/// New uncompleted future with same type.
///
/// In same zone or other [zone] if specified.
_Future<T> _newFutureWithSameType([_Zone? zone]) =>
_Future<T>.zone(zone ?? _zone);

@pragma("vm:entry-point")
Future<T> timeout(Duration timeLimit, {FutureOr<T> onTimeout()?}) {
if (_isComplete) return new _Future.immediate(this);
Expand Down Expand Up @@ -1041,3 +1049,7 @@ Function _registerErrorHandler(Function errorHandler, Zone zone) {
"Error handler must accept one Object or one Object and a StackTrace"
" as arguments, and return a value of the returned future's type");
}

// Names for positional arguments to _chainCoreFuture.
const _allowCompleteSync = true;
const _requireCompleteAsync = false;
50 changes: 50 additions & 0 deletions tests/lib/async/ignore_regress_56806_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright (c) 2024, 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.

import "dart:async";

import 'package:async_helper/async_helper.dart';
import "package:expect/expect.dart";

// Regression test for http://dartbug.com/56806
//
// Checks that `.ignore()` gets propagated correctly when chaining futures.

void main() async {
asyncStart();

await testNoThrow();

asyncStart();
await runZoned(testDoThrow, zoneSpecification: ZoneSpecification(
handleUncaughtError: (s, p, z, Object error, StackTrace stack) {
asyncEnd();
}));

asyncEnd();
}

Future<void> testNoThrow() async {
var completer = Completer<void>();
final future = Future<void>.delayed(
Duration.zero, () => throw StateError("Should be ignored"));
var future2 = future.whenComplete(() async {
await Future.delayed(Duration.zero);
completer.complete();
});
future2.ignore(); // Has no listeners.
await completer.future;
}

Future<void> testDoThrow() async {
var completer = Completer<void>();
final future = Future<void>.delayed(
Duration.zero, () => throw StateError("Should not be ignored"));
future.ignore(); // Ignores error only on `future`.
future.whenComplete(() async {
await Future.delayed(Duration.zero);
completer.complete();
}); // Should rethrow the error, as uncaught.
await completer.future;
}

0 comments on commit 6f1b279

Please sign in to comment.