diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 5835283cc58..f7d5d4ea950 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -1,4 +1,4 @@ -import { strictEqual, ok, deepStrictEqual } from 'node:assert'; +import { strictEqual, ok, deepStrictEqual, rejects } from 'node:assert'; const enc = new TextEncoder(); @@ -466,6 +466,116 @@ export const readableStreamFromNoopAsyncGen = { }, }; +// Tests for ReadableStream.from() cancel behavior per WPT spec +export const readableStreamFromCancelRejectsWhenReturnRejects = { + async test() { + const rejectError = new Error('return error'); + const iterable = { + async next() { + return { value: undefined, done: true }; + }, + async return() { + throw rejectError; + }, + [Symbol.asyncIterator]() { + return this; + }, + }; + + const rs = ReadableStream.from(iterable); + const reader = rs.getReader(); + + await rejects(reader.cancel(), rejectError); + }, +}; + +export const readableStreamFromCancelRejectsWhenReturnThrows = { + async test() { + const throwError = new Error('return throws'); + const iterable = { + async next() { + return { value: undefined, done: true }; + }, + return() { + throw throwError; + }, + [Symbol.asyncIterator]() { + return this; + }, + }; + + const rs = ReadableStream.from(iterable); + const reader = rs.getReader(); + + await rejects(reader.cancel(), (err) => err === throwError); + }, +}; + +export const readableStreamFromCancelRejectsWhenReturnNotMethod = { + async test() { + const iterable = { + async next() { + return { value: undefined, done: true }; + }, + return: 42, // exists but not callable + [Symbol.asyncIterator]() { + return this; + }, + }; + + const rs = ReadableStream.from(iterable); + const reader = rs.getReader(); + + await rejects(reader.cancel(), { + name: 'TypeError', + message: /return/, + }); + }, +}; + +export const readableStreamFromCancelRejectsWhenReturnNonObject = { + async test() { + const iterable = { + async next() { + return { value: undefined, done: true }; + }, + async return() { + return 42; // fulfills with non-object + }, + [Symbol.asyncIterator]() { + return this; + }, + }; + + const rs = ReadableStream.from(iterable); + const reader = rs.getReader(); + + await rejects(reader.cancel(), { + name: 'TypeError', + }); + }, +}; + +export const readableStreamFromCancelResolvesWhenReturnMissing = { + async test() { + const iterable = { + async next() { + return { value: undefined, done: true }; + }, + // no return method + [Symbol.asyncIterator]() { + return this; + }, + }; + + const rs = ReadableStream.from(iterable); + const reader = rs.getReader(); + + // Should resolve without error when return() is missing + await Promise.all([reader.cancel(), reader.closed]); + }, +}; + export const abortWriterAfterGc = { async test() { function getWriter() { diff --git a/src/workerd/jsg/iterator.h b/src/workerd/jsg/iterator.h index 7050ca18a51..b32ea6aeaa4 100644 --- a/src/workerd/jsg/iterator.h +++ b/src/workerd/jsg/iterator.h @@ -205,8 +205,17 @@ class AsyncGenerator final { } // If nothing is returned, the generator is complete. + // Per GetMethod spec (https://262.ecma-international.org/#sec-getmethod), if the 'return' + // property exists but is not callable, we throw a TypeError. Promise> return_(Lock& js, kj::Maybe maybeValue = kj::none) { KJ_IF_SOME(active, maybeActive) { + // Per GetMethod spec: if property exists but is not callable, throw TypeError + if (active.returnExistsButNotCallable) { + maybeActive = kj::none; + return js.rejectedPromise>( + js.typeError("Property 'return' is not a function"_kj)); + } + KJ_IF_SOME(return_, active.maybeReturn) { auto& selfRef = KJ_ASSERT_NONNULL(maybeSelfRef); return js.tryCatch([&] { @@ -217,17 +226,13 @@ class AsyncGenerator final { } return js.resolvedPromise(kj::mv(result.value)); }, [ref = selfRef.addRef()](Lock& js, Value exception) { - Promise> retPromise = nullptr; - if (ref->runIfAlive([&](AsyncGenerator& self) { - retPromise = self.throw_(js, kj::mv(exception)); - })) { - return kj::mv(retPromise); - } + // Per spec, rejections from return() should be propagated directly + ref->runIfAlive([&](AsyncGenerator& self) { self.maybeActive = kj::none; }); return js.rejectedPromise>(kj::mv(exception)); }); }, [&](Value exception) { maybeActive = kj::none; - return throw_(js, kj::mv(exception)); + return js.rejectedPromise>(kj::mv(exception)); }); } maybeActive = kj::none; @@ -276,6 +281,9 @@ class AsyncGenerator final { kj::Maybe maybeNext; kj::Maybe maybeReturn; kj::Maybe maybeThrow; + // Per GetMethod spec, if property exists but is not callable, we should throw TypeError. + // We track this state to defer the error to when return_() is actually called. + bool returnExistsButNotCallable = false; template Active(Lock& js, JsObject object, TypeWrapper*) @@ -283,6 +291,9 @@ class AsyncGenerator final { maybeReturn( tryGetGeneratorFunction(js, object, "return"_kj)), maybeThrow(tryGetGeneratorFunction(js, object, "throw"_kj)) { + // Check if return property exists but isn't callable (per GetMethod spec) + returnExistsButNotCallable = + maybeReturn == kj::none && !object.get(js, "return"_kj).isNullOrUndefined(); } Active(Active&&) = default; Active& operator=(Active&&) = default; diff --git a/src/wpt/streams-test.ts b/src/wpt/streams-test.ts index cf32cfef342..5e4280da98e 100644 --- a/src/wpt/streams-test.ts +++ b/src/wpt/streams-test.ts @@ -422,19 +422,7 @@ export default { 'Floating point arithmetic must manifest near 0 (total ends up zero)', ], }, - 'readable-streams/from.any.js': { - comment: 'See comments on tests', - disabledTests: [ - // A hanging promise was cancelled - 'ReadableStream.from: cancel() rejects when return() rejects', - 'ReadableStream.from: cancel() rejects when return() fulfills with a non-object', - ], - expectedFailures: [ - // TODO(soon): This one is a bit pedantic. We ignore the case where return() is not - // a method whereas the spec expects us to return a rejected promise in this case. - 'ReadableStream.from: cancel() rejects when return() is not a method', - ], - }, + 'readable-streams/from.any.js': {}, 'readable-streams/garbage-collection.any.js': { comment: 'See comments on individual tests', disabledTests: [