LibJS: Close sync iterator when async wrapper yields rejection

This is a normative change in the ECMA-262 spec. See:
ff129b1

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
+enum class CloseOnRejection {
-static Object* async_from_sync_iterator_continuation(VM& vm, Object& result, PromiseCapability& promise_capability)
+    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).
+    // 6. Let valueWrapper be Completion(PromiseResolve(%Promise%, value)).
-    // 7. IfAbruptRejectPromise(valueWrapper, promiseCapability).
+    auto value_wrapper_completion = [&]() -> ThrowCompletionOr<JS::Value> {
-    auto value_wrapper = TRY_OR_MUST_REJECT(vm, &promise_capability, promise_resolve(vm, realm.intrinsics().promise_constructor(), value));
+        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<Value> {
         // a. Return CreateIterResultObject(value, done).
         return create_iterator_result_object(vm, vm.argument(0), done).ptr();
     };
 
-    // 9. Let onFulfilled be CreateBuiltinFunction(unwrap, 1, "", « »).
+    // 10. 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.
+    // 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).
+    Value on_rejected;
-    as<Promise>(value_wrapper)->perform_then(move(on_fulfilled), js_undefined(), &promise_capability);
 
-    // 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<Value> {
+            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<Promise>(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).
+    // 8. Return AsyncFromSyncIteratorContinuation(result, promiseCapability, syncIteratorRecord, true).
-    return async_from_sync_iterator_continuation(vm, result, promise_capability);
+    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]].
+    // 4. Let syncIteratorRecord be O.[[SyncIteratorRecord]].
-    auto sync_iterator = this_object->sync_iterator_record().iterator;
+    auto& sync_iterator_record = this_object->sync_iterator_record();
 
-    // 5. Let return be Completion(GetMethod(syncIterator, "return")).
+    // 5. Let syncIterator be syncIteratorRecord.[[Iterator]].
-    // 6. IfAbruptRejectPromise(return, promiseCapability).
+    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).
+        // a. Let iteratorResult be CreateIteratorResultObject(value, true).
-        auto iter_result = create_iterator_result_object(vm, vm.argument(0), true);
+        auto iterator_result = create_iterator_result_object(vm, vm.argument(0), true);
 
-        // b. Perform ! Call(promiseCapability.[[Resolve]], undefined, « iterResult »).
+        // b. Perform ! Call(promiseCapability.[[Resolve]], undefined, « iteratorResult »).
-        MUST(call(vm, *promise_capability->resolve(), js_undefined(), iter_result));
+        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).
+    // 13. Return AsyncFromSyncIteratorContinuation(result, promiseCapability, syncIteratorRecord, false).
-    return async_from_sync_iterator_continuation(vm, result.as_object(), promise_capability);
+    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]].
+    // 4. Let syncIteratorRecord be O.[[SyncIteratorRecord]].
-    auto sync_iterator = this_object->sync_iterator_record().iterator;
+    auto& sync_iterator_record = this_object->sync_iterator_record();
 
-    // 5. Let throw be Completion(GetMethod(syncIterator, "throw")).
+    // 5. Let syncIterator be syncIteratorRecord.[[Iterator]].
-    // 6. IfAbruptRejectPromise(throw, promiseCapability).
+    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 »).
+        // 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.
-        MUST(call(vm, *promise_capability->reject(), js_undefined(), vm.argument(0)));
+
-        // b. Return promiseCapability.[[Promise]].
+        // 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).
+    // 13. Return AsyncFromSyncIteratorContinuation(result, promiseCapability, syncIteratorRecord, true).
-    return async_from_sync_iterator_continuation(vm, result.as_object(), promise_capability);
+    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

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);
+    });
 });

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)");