From c04144696f35df3b614fe69865a2673daaee61bc Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:19:30 -0500 Subject: [PATCH 1/9] fix ReadableStream.from() cancel behavior per WPT spec --- src/workerd/api/tests/streams-test.js | 113 +++++++++++++++++++++++++- src/workerd/jsg/iterator.h | 25 ++++-- src/wpt/streams-test.ts | 14 +--- 3 files changed, 131 insertions(+), 21 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 5835283cc58..d6ace58ce8d 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,117 @@ 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 = { + next() { + return Promise.resolve({ value: undefined, done: true }); + }, + return() { + return Promise.reject(rejectError); + }, + [Symbol.asyncIterator]() { + return this; + }, + }; + + const rs = ReadableStream.from(iterable); + const reader = rs.getReader(); + + await rejects(reader.cancel(), (err) => err === rejectError); + }, +}; + +export const readableStreamFromCancelRejectsWhenReturnThrows = { + async test() { + const throwError = new Error('return throws'); + const iterable = { + next() { + return Promise.resolve({ 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 = { + next() { + return Promise.resolve({ 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 = { + next() { + return Promise.resolve({ value: undefined, done: true }); + }, + return() { + return Promise.resolve(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 = { + next() { + return Promise.resolve({ 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 reader.cancel(); + await 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..12e94c5c10c 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: [ From d62fb7798c21bad6d9e9e4c4ac86bf1d46784ab1 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:24:32 -0500 Subject: [PATCH 2/9] Update src/workerd/api/tests/streams-test.js Co-authored-by: James M Snell --- src/workerd/api/tests/streams-test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index d6ace58ce8d..4b76bd8e04d 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -471,11 +471,11 @@ export const readableStreamFromCancelRejectsWhenReturnRejects = { async test() { const rejectError = new Error('return error'); const iterable = { - next() { - return Promise.resolve({ value: undefined, done: true }); + async next() { + return { value: undefined, done: true }; }, - return() { - return Promise.reject(rejectError); + async return() { + throw rejectError; }, [Symbol.asyncIterator]() { return this; From b706464b92663f33f0ae578543f840b507dabde2 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:25:09 -0500 Subject: [PATCH 3/9] Update src/workerd/api/tests/streams-test.js Co-authored-by: James M Snell --- src/workerd/api/tests/streams-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 4b76bd8e04d..03197cabdb5 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -493,8 +493,8 @@ export const readableStreamFromCancelRejectsWhenReturnThrows = { async test() { const throwError = new Error('return throws'); const iterable = { - next() { - return Promise.resolve({ value: undefined, done: true }); + async next() { + return { value: undefined, done: true }; }, return() { throw throwError; From c8c314bb68314f56593b0399c4832999464d812f Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:26:11 -0500 Subject: [PATCH 4/9] Update src/workerd/api/tests/streams-test.js Co-authored-by: James M Snell --- src/workerd/api/tests/streams-test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 03197cabdb5..42c832c08d5 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -536,11 +536,11 @@ export const readableStreamFromCancelRejectsWhenReturnNotMethod = { export const readableStreamFromCancelRejectsWhenReturnNonObject = { async test() { const iterable = { - next() { - return Promise.resolve({ value: undefined, done: true }); + async next() { + return { value: undefined, done: true }; }, - return() { - return Promise.resolve(42); // fulfills with non-object + async return() { + return 42; // fulfills with non-object }, [Symbol.asyncIterator]() { return this; From d19b8f2f7b8d3140714a102160d1864f4cc1e5cb Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:26:17 -0500 Subject: [PATCH 5/9] Update src/workerd/api/tests/streams-test.js Co-authored-by: James M Snell --- src/workerd/api/tests/streams-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 42c832c08d5..0796a60e9f7 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -514,8 +514,8 @@ export const readableStreamFromCancelRejectsWhenReturnThrows = { export const readableStreamFromCancelRejectsWhenReturnNotMethod = { async test() { const iterable = { - next() { - return Promise.resolve({ value: undefined, done: true }); + async next() { + return { value: undefined, done: true }; }, return: 42, // exists but not callable [Symbol.asyncIterator]() { From 723a48aac430af2f20e799098184e080bb96f6cf Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:27:18 -0500 Subject: [PATCH 6/9] Update src/workerd/api/tests/streams-test.js --- src/workerd/api/tests/streams-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 0796a60e9f7..8eec5a2baf3 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -485,7 +485,7 @@ export const readableStreamFromCancelRejectsWhenReturnRejects = { const rs = ReadableStream.from(iterable); const reader = rs.getReader(); - await rejects(reader.cancel(), (err) => err === rejectError); + await rejects(reader.cancel(), rejectError); }, }; From e42d5b93219761f6f4509059170f1c59fc0d69c7 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:27:29 -0500 Subject: [PATCH 7/9] Update src/workerd/api/tests/streams-test.js Co-authored-by: James M Snell --- src/workerd/api/tests/streams-test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index 8eec5a2baf3..dd41fe3aa41 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -572,8 +572,7 @@ export const readableStreamFromCancelResolvesWhenReturnMissing = { const reader = rs.getReader(); // Should resolve without error when return() is missing - await reader.cancel(); - await reader.closed; + await Promise.all([reader.cancel(), reader.closed]); }, }; From 807915e47c6f0f828fbdbb8be58fcfd596f29b3a Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:27:35 -0500 Subject: [PATCH 8/9] Update src/workerd/api/tests/streams-test.js Co-authored-by: James M Snell --- src/workerd/api/tests/streams-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/workerd/api/tests/streams-test.js b/src/workerd/api/tests/streams-test.js index dd41fe3aa41..f7d5d4ea950 100644 --- a/src/workerd/api/tests/streams-test.js +++ b/src/workerd/api/tests/streams-test.js @@ -559,8 +559,8 @@ export const readableStreamFromCancelRejectsWhenReturnNonObject = { export const readableStreamFromCancelResolvesWhenReturnMissing = { async test() { const iterable = { - next() { - return Promise.resolve({ value: undefined, done: true }); + async next() { + return { value: undefined, done: true }; }, // no return method [Symbol.asyncIterator]() { From f39c0c7dac09804174714b0e7321097ed4bb848f Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 30 Dec 2025 10:29:35 -0500 Subject: [PATCH 9/9] Update src/workerd/jsg/iterator.h Co-authored-by: James M Snell --- src/workerd/jsg/iterator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workerd/jsg/iterator.h b/src/workerd/jsg/iterator.h index 12e94c5c10c..b32ea6aeaa4 100644 --- a/src/workerd/jsg/iterator.h +++ b/src/workerd/jsg/iterator.h @@ -213,7 +213,7 @@ class AsyncGenerator final { if (active.returnExistsButNotCallable) { maybeActive = kj::none; return js.rejectedPromise>( - js.typeError("property 'return' is not a function"_kj)); + js.typeError("Property 'return' is not a function"_kj)); } KJ_IF_SOME(return_, active.maybeReturn) {