Skip to content

Commit 59907b7

Browse files
committed
fix ReadableStream.from() cancel behavior per WPT spec
1 parent 33c030d commit 59907b7

File tree

2 files changed

+19
-20
lines changed

2 files changed

+19
-20
lines changed

src/workerd/jsg/iterator.h

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,17 @@ class AsyncGenerator final {
205205
}
206206

207207
// If nothing is returned, the generator is complete.
208+
// Per GetMethod spec (https://262.ecma-international.org/#sec-getmethod), if the 'return'
209+
// property exists but is not callable, we throw a TypeError.
208210
Promise<kj::Maybe<T>> return_(Lock& js, kj::Maybe<T> maybeValue = kj::none) {
209211
KJ_IF_SOME(active, maybeActive) {
212+
// Per GetMethod spec: if property exists but is not callable, throw TypeError
213+
if (active.returnExistsButNotCallable) {
214+
maybeActive = kj::none;
215+
return js.rejectedPromise<kj::Maybe<T>>(
216+
js.typeError("property 'return' is not a function"_kj));
217+
}
218+
210219
KJ_IF_SOME(return_, active.maybeReturn) {
211220
auto& selfRef = KJ_ASSERT_NONNULL(maybeSelfRef);
212221
return js.tryCatch([&] {
@@ -217,17 +226,13 @@ class AsyncGenerator final {
217226
}
218227
return js.resolvedPromise(kj::mv(result.value));
219228
}, [ref = selfRef.addRef()](Lock& js, Value exception) {
220-
Promise<kj::Maybe<T>> retPromise = nullptr;
221-
if (ref->runIfAlive([&](AsyncGenerator& self) {
222-
retPromise = self.throw_(js, kj::mv(exception));
223-
})) {
224-
return kj::mv(retPromise);
225-
}
229+
// Per spec, rejections from return() should be propagated directly
230+
ref->runIfAlive([&](AsyncGenerator& self) { self.maybeActive = kj::none; });
226231
return js.rejectedPromise<kj::Maybe<T>>(kj::mv(exception));
227232
});
228233
}, [&](Value exception) {
229234
maybeActive = kj::none;
230-
return throw_(js, kj::mv(exception));
235+
return js.rejectedPromise<kj::Maybe<T>>(kj::mv(exception));
231236
});
232237
}
233238
maybeActive = kj::none;
@@ -276,13 +281,19 @@ class AsyncGenerator final {
276281
kj::Maybe<NextSignature> maybeNext;
277282
kj::Maybe<ReturnSignature> maybeReturn;
278283
kj::Maybe<ThrowSignature> maybeThrow;
284+
// Per GetMethod spec, if property exists but is not callable, we should throw TypeError.
285+
// We track this state to defer the error to when return_() is actually called.
286+
bool returnExistsButNotCallable = false;
279287

280288
template <typename TypeWrapper>
281289
Active(Lock& js, JsObject object, TypeWrapper*)
282290
: maybeNext(tryGetGeneratorFunction<NextSignature, TypeWrapper>(js, object, "next"_kj)),
283291
maybeReturn(
284292
tryGetGeneratorFunction<ReturnSignature, TypeWrapper>(js, object, "return"_kj)),
285293
maybeThrow(tryGetGeneratorFunction<ThrowSignature, TypeWrapper>(js, object, "throw"_kj)) {
294+
// Check if return property exists but isn't callable (per GetMethod spec)
295+
returnExistsButNotCallable =
296+
maybeReturn == kj::none && !object.get(js, "return"_kj).isNullOrUndefined();
286297
}
287298
Active(Active&&) = default;
288299
Active& operator=(Active&&) = default;

src/wpt/streams-test.ts

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -422,19 +422,7 @@ export default {
422422
'Floating point arithmetic must manifest near 0 (total ends up zero)',
423423
],
424424
},
425-
'readable-streams/from.any.js': {
426-
comment: 'See comments on tests',
427-
disabledTests: [
428-
// A hanging promise was cancelled
429-
'ReadableStream.from: cancel() rejects when return() rejects',
430-
'ReadableStream.from: cancel() rejects when return() fulfills with a non-object',
431-
],
432-
expectedFailures: [
433-
// TODO(soon): This one is a bit pedantic. We ignore the case where return() is not
434-
// a method whereas the spec expects us to return a rejected promise in this case.
435-
'ReadableStream.from: cancel() rejects when return() is not a method',
436-
],
437-
},
425+
'readable-streams/from.any.js': {},
438426
'readable-streams/garbage-collection.any.js': {
439427
comment: 'See comments on individual tests',
440428
disabledTests: [

0 commit comments

Comments
 (0)