From 568524f8ba945967c4d63022db1a06baf2b32ce7 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Mon, 28 Apr 2025 17:53:04 -0400 Subject: [PATCH] LibJS: Close sync iterator when async wrapper yields rejection This is a normative change in the ECMA-262 spec. See: https://github.com/tc39/ecma262/commit/ff129b1 --- .../AsyncFromSyncIteratorPrototype.cpp | 152 ++++++++++++------ .../Tests/builtins/Array/Array.fromAsync.js | 24 +++ Libraries/LibJS/Tests/test-common.js | 31 ++++ 3 files changed, 162 insertions(+), 45 deletions(-) diff --git a/Libraries/LibJS/Runtime/AsyncFromSyncIteratorPrototype.cpp b/Libraries/LibJS/Runtime/AsyncFromSyncIteratorPrototype.cpp index b8624ab376d..690f60b9721 100644 --- a/Libraries/LibJS/Runtime/AsyncFromSyncIteratorPrototype.cpp +++ b/Libraries/LibJS/Runtime/AsyncFromSyncIteratorPrototype.cpp @@ -32,12 +32,19 @@ void AsyncFromSyncIteratorPrototype::initialize(Realm& realm) define_native_function(realm, vm.names.throw_, throw_, 1, attr); } -// 27.1.4.4 AsyncFromSyncIteratorContinuation ( result, promiseCapability ), https://tc39.es/ecma262/#sec-asyncfromsynciteratorcontinuation -static Object* async_from_sync_iterator_continuation(VM& vm, Object& result, PromiseCapability& promise_capability) +enum class CloseOnRejection { + No, + Yes, +}; + +// 27.1.4.4 AsyncFromSyncIteratorContinuation ( result, promiseCapability, syncIteratorRecord, closeOnRejection ), https://tc39.es/ecma262/#sec-asyncfromsynciteratorcontinuation +static Object* async_from_sync_iterator_continuation(VM& vm, Object& result, PromiseCapability& promise_capability, IteratorRecord const& sync_iterator_record, CloseOnRejection close_on_rejection) { auto& realm = *vm.current_realm(); - // 1. NOTE: Because promiseCapability is derived from the intrinsic %Promise%, the calls to promiseCapability.[[Reject]] entailed by the use IfAbruptRejectPromise below are guaranteed not to throw. + // 1. NOTE: Because promiseCapability is derived from the intrinsic %Promise%, the calls to promiseCapability.[[Reject]] + // entailed by the use IfAbruptRejectPromise below are guaranteed not to throw. + // 2. Let done be Completion(IteratorComplete(result)). // 3. IfAbruptRejectPromise(done, promiseCapability). auto done = TRY_OR_MUST_REJECT(vm, &promise_capability, iterator_complete(vm, result)); @@ -46,24 +53,58 @@ static Object* async_from_sync_iterator_continuation(VM& vm, Object& result, Pro // 5. IfAbruptRejectPromise(value, promiseCapability). auto value = TRY_OR_MUST_REJECT(vm, &promise_capability, iterator_value(vm, result)); - // 6. Let valueWrapper be PromiseResolve(%Promise%, value). - // 7. IfAbruptRejectPromise(valueWrapper, promiseCapability). - auto value_wrapper = TRY_OR_MUST_REJECT(vm, &promise_capability, promise_resolve(vm, realm.intrinsics().promise_constructor(), value)); + // 6. Let valueWrapper be Completion(PromiseResolve(%Promise%, value)). + auto value_wrapper_completion = [&]() -> ThrowCompletionOr { + return TRY(promise_resolve(vm, realm.intrinsics().promise_constructor(), value)); + }(); - // 8. Let unwrap be a new Abstract Closure with parameters (value) that captures done and performs the following steps when called: + // 7. If valueWrapper is an abrupt completion, done is false, and closeOnRejection is true, then + if (value_wrapper_completion.is_error() && !done && close_on_rejection == CloseOnRejection::Yes) { + // a. Set valueWrapper to Completion(IteratorClose(syncIteratorRecord, valueWrapper)). + value_wrapper_completion = iterator_close(vm, sync_iterator_record, value_wrapper_completion); + } + + // 8. IfAbruptRejectPromise(valueWrapper, promiseCapability). + auto value_wrapper = TRY_OR_MUST_REJECT(vm, &promise_capability, value_wrapper_completion); + + // 9. Let unwrap be a new Abstract Closure with parameters (value) that captures done and performs the following steps when called: auto unwrap = [done](VM& vm) -> ThrowCompletionOr { // a. Return CreateIterResultObject(value, done). return create_iterator_result_object(vm, vm.argument(0), done).ptr(); }; - // 9. Let onFulfilled be CreateBuiltinFunction(unwrap, 1, "", « »). - // 10. NOTE: onFulfilled is used when processing the "value" property of an IteratorResult object in order to wait for its value if it is a promise and re-package the result in a new "unwrapped" IteratorResult object. + // 10. Let onFulfilled be CreateBuiltinFunction(unwrap, 1, "", « »). + // 11. NOTE: onFulfilled is used when processing the "value" property of an IteratorResult object in order to wait for its value if it is a promise and re-package the result in a new "unwrapped" IteratorResult object. auto on_fulfilled = NativeFunction::create(realm, move(unwrap), 1); - // 11. Perform PerformPromiseThen(valueWrapper, onFulfilled, undefined, promiseCapability). - as(value_wrapper)->perform_then(move(on_fulfilled), js_undefined(), &promise_capability); + Value on_rejected; - // 12. Return promiseCapability.[[Promise]]. + // 12. If done is true, or if closeOnRejection is false, then + if (done || close_on_rejection == CloseOnRejection::No) { + // a. Let onRejected be undefined. + on_rejected = js_undefined(); + } + // 13. Else, + else { + // a. Let closeIterator be a new Abstract Closure with parameters (error) that captures syncIteratorRecord and performs the following steps when called: + auto close_iterator = [&sync_iterator_record](VM& vm) -> ThrowCompletionOr { + auto error = vm.argument(0); + + // i. Return ? IteratorClose(syncIteratorRecord, ThrowCompletion(error)). + return iterator_close(vm, sync_iterator_record, throw_completion(error)); + }; + + // b. Let onRejected be CreateBuiltinFunction(closeIterator, 1, "", « »). + on_rejected = NativeFunction::create(realm, move(close_iterator), 1); + + // c. NOTE: onRejected is used to close the Iterator when the "value" property of an IteratorResult object it + // yields is a rejected promise. + } + + // 14. Perform PerformPromiseThen(valueWrapper, onFulfilled, onRejected, promiseCapability). + as(value_wrapper.as_object()).perform_then(on_fulfilled, on_rejected, promise_capability); + + // 15. Return promiseCapability.[[Promise]]. return promise_capability.promise(); } @@ -91,8 +132,8 @@ JS_DEFINE_NATIVE_FUNCTION(AsyncFromSyncIteratorPrototype::next) (vm.argument_count() > 0 ? iterator_next(vm, sync_iterator_record, vm.argument(0)) : iterator_next(vm, sync_iterator_record))); - // 8. Return AsyncFromSyncIteratorContinuation(result, promiseCapability). - return async_from_sync_iterator_continuation(vm, result, promise_capability); + // 8. Return AsyncFromSyncIteratorContinuation(result, promiseCapability, syncIteratorRecord, true). + return async_from_sync_iterator_continuation(vm, result, promise_capability, sync_iterator_record, CloseOnRejection::Yes); } // 27.1.4.2.2 %AsyncFromSyncIteratorPrototype%.return ( [ value ] ), https://tc39.es/ecma262/#sec-%asyncfromsynciteratorprototype%.return @@ -107,45 +148,49 @@ JS_DEFINE_NATIVE_FUNCTION(AsyncFromSyncIteratorPrototype::return_) // 3. Let promiseCapability be ! NewPromiseCapability(%Promise%). auto promise_capability = MUST(new_promise_capability(vm, realm.intrinsics().promise_constructor())); - // 4. Let syncIterator be O.[[SyncIteratorRecord]].[[Iterator]]. - auto sync_iterator = this_object->sync_iterator_record().iterator; + // 4. Let syncIteratorRecord be O.[[SyncIteratorRecord]]. + auto& sync_iterator_record = this_object->sync_iterator_record(); - // 5. Let return be Completion(GetMethod(syncIterator, "return")). - // 6. IfAbruptRejectPromise(return, promiseCapability). + // 5. Let syncIterator be syncIteratorRecord.[[Iterator]]. + auto sync_iterator = sync_iterator_record.iterator; + + // 6. Let return be Completion(GetMethod(syncIterator, "return")). + // 7. IfAbruptRejectPromise(return, promiseCapability). auto return_method = TRY_OR_REJECT(vm, promise_capability, Value(sync_iterator).get_method(vm, vm.names.return_)); - // 7. If return is undefined, then + // 8. If return is undefined, then if (return_method == nullptr) { - // a. Let iterResult be CreateIterResultObject(value, true). - auto iter_result = create_iterator_result_object(vm, vm.argument(0), true); + // a. Let iteratorResult be CreateIteratorResultObject(value, true). + auto iterator_result = create_iterator_result_object(vm, vm.argument(0), true); - // b. Perform ! Call(promiseCapability.[[Resolve]], undefined, « iterResult »). - MUST(call(vm, *promise_capability->resolve(), js_undefined(), iter_result)); + // b. Perform ! Call(promiseCapability.[[Resolve]], undefined, « iteratorResult »). + MUST(call(vm, *promise_capability->resolve(), js_undefined(), iterator_result)); // c. Return promiseCapability.[[Promise]]. return promise_capability->promise(); } - // 8. If value is present, then + // 9. If value is present, then // a. Let result be Completion(Call(return, syncIterator, « value »)). - // 9. Else, + // 10. Else, // a. Let result be Completion(Call(return, syncIterator)). - // 10. IfAbruptRejectPromise(result, promiseCapability). + // 11. IfAbruptRejectPromise(result, promiseCapability). auto result = TRY_OR_REJECT(vm, promise_capability, (vm.argument_count() > 0 ? call(vm, return_method, sync_iterator, vm.argument(0)) : call(vm, return_method, sync_iterator))); - // 11. If Type(result) is not Object, then + // 12. If Type(result) is not Object, then if (!result.is_object()) { auto error = TypeError::create(realm, TRY_OR_THROW_OOM(vm, String::formatted(ErrorType::NotAnObject.message(), "SyncIteratorReturnResult"))); // a. Perform ! Call(promiseCapability.[[Reject]], undefined, « a newly created TypeError object »). MUST(call(vm, *promise_capability->reject(), js_undefined(), error)); + // b. Return promiseCapability.[[Promise]]. return promise_capability->promise(); } - // 12. Return AsyncFromSyncIteratorContinuation(result, promiseCapability). - return async_from_sync_iterator_continuation(vm, result.as_object(), promise_capability); + // 13. Return AsyncFromSyncIteratorContinuation(result, promiseCapability, syncIteratorRecord, false). + return async_from_sync_iterator_continuation(vm, result.as_object(), promise_capability, sync_iterator_record, CloseOnRejection::No); } // 27.1.4.2.3 %AsyncFromSyncIteratorPrototype%.throw ( [ value ] ), https://tc39.es/ecma262/#sec-%asyncfromsynciteratorprototype%.throw @@ -160,42 +205,59 @@ JS_DEFINE_NATIVE_FUNCTION(AsyncFromSyncIteratorPrototype::throw_) // 3. Let promiseCapability be ! NewPromiseCapability(%Promise%). auto promise_capability = MUST(new_promise_capability(vm, realm.intrinsics().promise_constructor())); - // 4. Let syncIterator be O.[[SyncIteratorRecord]].[[Iterator]]. - auto sync_iterator = this_object->sync_iterator_record().iterator; + // 4. Let syncIteratorRecord be O.[[SyncIteratorRecord]]. + auto& sync_iterator_record = this_object->sync_iterator_record(); - // 5. Let throw be Completion(GetMethod(syncIterator, "throw")). - // 6. IfAbruptRejectPromise(throw, promiseCapability). + // 5. Let syncIterator be syncIteratorRecord.[[Iterator]]. + auto sync_iterator = sync_iterator_record.iterator; + + // 6. Let throw be Completion(GetMethod(syncIterator, "throw")). + // 7. IfAbruptRejectPromise(throw, promiseCapability). auto throw_method = TRY_OR_REJECT(vm, promise_capability, Value(sync_iterator).get_method(vm, vm.names.throw_)); - // 7. If throw is undefined, then + // 8. If throw is undefined, then if (throw_method == nullptr) { - // a. Perform ! Call(promiseCapability.[[Reject]], undefined, « value »). - MUST(call(vm, *promise_capability->reject(), js_undefined(), vm.argument(0))); - // b. Return promiseCapability.[[Promise]]. + // a. NOTE: If syncIterator does not have a throw method, close it to give it a chance to clean up before we reject the capability. + + // b. Let closeCompletion be NormalCompletion(empty). + auto close_completion = normal_completion({}); + + // c. Let result be Completion(IteratorClose(syncIteratorRecord, closeCompletion)). + // d. IfAbruptRejectPromise(result, promiseCapability). + TRY_OR_REJECT(vm, promise_capability, iterator_close(vm, sync_iterator_record, close_completion)); + + // e. NOTE: The next step throws a TypeError to indicate that there was a protocol violation: syncIterator does not have a throw method. + // f. NOTE: If closing syncIterator does not throw then the result of that operation is ignored, even if it yields a rejected promise. + + // g. Perform ! Call(promiseCapability.[[Reject]], undefined, « a newly created TypeError object »). + auto error = TypeError::create(realm, MUST(String::formatted(ErrorType::IsUndefined.message(), "throw method"))); + MUST(call(vm, *promise_capability->reject(), js_undefined(), error)); + + // h. Return promiseCapability.[[Promise]]. return promise_capability->promise(); } - // 8. If value is present, then + + // 9. If value is present, then // a. Let result be Completion(Call(throw, syncIterator, « value »)). - // 9. Else, + // 10. Else, // a. Let result be Completion(Call(throw, syncIterator)). - // 10. IfAbruptRejectPromise(result, promiseCapability). + // 11. IfAbruptRejectPromise(result, promiseCapability). auto result = TRY_OR_REJECT(vm, promise_capability, (vm.argument_count() > 0 ? call(vm, throw_method, sync_iterator, vm.argument(0)) : call(vm, throw_method, sync_iterator))); - // 11. If Type(result) is not Object, then + // 12. If result is not an Object, then if (!result.is_object()) { - auto error = TypeError::create(realm, TRY_OR_THROW_OOM(vm, String::formatted(ErrorType::NotAnObject.message(), "SyncIteratorThrowResult"))); - // a. Perform ! Call(promiseCapability.[[Reject]], undefined, « a newly created TypeError object »). + auto error = TypeError::create(realm, MUST(String::formatted(ErrorType::NotAnObject.message(), "SyncIteratorThrowResult"))); MUST(call(vm, *promise_capability->reject(), js_undefined(), error)); // b. Return promiseCapability.[[Promise]]. return promise_capability->promise(); } - // 12. Return AsyncFromSyncIteratorContinuation(result, promiseCapability). - return async_from_sync_iterator_continuation(vm, result.as_object(), promise_capability); + // 13. Return AsyncFromSyncIteratorContinuation(result, promiseCapability, syncIteratorRecord, true). + return async_from_sync_iterator_continuation(vm, result.as_object(), promise_capability, sync_iterator_record, CloseOnRejection::Yes); } // 27.1.4.1 CreateAsyncFromSyncIterator ( syncIteratorRecord ), https://tc39.es/ecma262/#sec-createasyncfromsynciterator diff --git a/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js b/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js index 65c106bd6c7..b0135e6b94e 100644 --- a/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js +++ b/Libraries/LibJS/Tests/builtins/Array/Array.fromAsync.js @@ -77,4 +77,28 @@ describe("normal behavior", () => { checkResult(promise, TestArray); expect(callCount).toBe(1); }); + + asyncTest("sync iterable is closed upon rejection", async () => { + const thenable = { + then(resolve, reject) { + reject(); + }, + }; + + let counter = 0; + + function* iterator() { + try { + yield thenable; + } finally { + counter++; + } + } + + try { + await Array.fromAsync(iterator()); + } catch (e) {} + + expect(counter).toBe(1); + }); }); diff --git a/Libraries/LibJS/Tests/test-common.js b/Libraries/LibJS/Tests/test-common.js index e020dee48b4..30977525f78 100644 --- a/Libraries/LibJS/Tests/test-common.js +++ b/Libraries/LibJS/Tests/test-common.js @@ -617,6 +617,37 @@ class ExpectationError extends Error { } }; + asyncTest = async (message, callback) => { + if (!__TestResults__[suiteMessage]) __TestResults__[suiteMessage] = {}; + + const suite = __TestResults__[suiteMessage]; + if (Object.prototype.hasOwnProperty.call(suite, message)) { + suite[message] = { + result: "fail", + details: "Another test with the same message did already run", + duration: 0, + }; + return; + } + + const start = Date.now(); + const time_ms = () => Date.now() - start; + + try { + await callback(); + suite[message] = { + result: "pass", + duration: time_ms(), + }; + } catch (e) { + suite[message] = { + result: "fail", + details: String(e), + duration: time_ms(), + }; + } + }; + test.skip = (message, callback) => { if (typeof callback !== "function") throw new Error("test.skip has invalid second argument (must be a function)");