ReadableStreamDefaultReader.read accesses Object.prototype.then

[KhafraDev]

Version

v19.7.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

web streams

What steps will reproduce the bug?

const hello = new TextEncoder().encode('hello')

const rs = new ReadableStream({
  start(controller) {
    controller.enqueue(hello);
    controller.close();
  }
})

const reader = rs.getReader()

Object.defineProperty(Object.prototype, 'then', {
  get () {
    throw new Error()
  }
})

while (true) {
  const { done } = await reader.read()

  if (done) break
}

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No error thrown

What do you see instead?

file:///undici/test.mjs:14
    throw new Error()
          ^

Error
    at Object.get (file:///undici/test.mjs:14:11)
    at Object.resolve (<anonymous>)
    at [kChunk] (node:internal/webstreams/readablestream:771:27)
    at readableStreamDefaultControllerPullSteps (node:internal/webstreams/readablestream:2304:24)
    at [kPull] (node:internal/webstreams/readablestream:1052:5)
    at readableStreamDefaultReaderRead (node:internal/webstreams/readablestream:2138:39)
    at ReadableStreamDefaultReader.read (node:internal/webstreams/readablestream:841:5)
    at file:///undici/test.mjs:19:33
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)

Additional information

This is a WPT that I discovered working on fetch, https://github.com/web-platform-tests/wpt/blob/master/fetch/api/response/response-stream-with-broken-then.any.js

[aduh95]

I think that's the expected behavior, all native async iterators behave like that AFAIK (in browsers as well). The reason is that CreateIterResultObject requires the result object to inherit from Object.prototype, and part of resolving the promise includes checking if the .then property of the result object (see https://tc39.es/ecma262/#sec-promise-resolve-functions step 9) is callable.

[panva]

@aduh95 the browsers pass these WPTs tho.

[aduh95]

The above code snippet rejects in Firefox and Chromium, passes on Safari AFAICT. I think Safari is incorrect here.

[KhafraDev]

Unfortunately there's no other way to read from a ReadableStream/ReadableStream reader, which the fetch spec requires us to do (see: https://streams.spec.whatwg.org/#readablestreamdefaultreader-read-all-bytes). Using its async iterator has the same issue.

[aduh95]

The spec says Perform ! [ReadableStreamDefaultReaderRead](https://streams.spec.whatwg.org/#readable-stream-default-reader-read)(reader, readRequest)., which is exported by lib/internal/webstreams:

node/lib/internal/webstreams/readablestream.js

Lines 2124 to 2140 in d42628d

	function readableStreamDefaultReaderRead(reader, readRequest) {
	const {
	stream,
	} = reader[kState];
	assert(stream !== undefined);
	stream[kState].disturbed = true;
	switch (stream[kState].state) {
	case 'closed':
	readRequest[kClose]();
	break;
	case 'errored':
	readRequest[kError](stream[kState].storedError);
	break;
	case 'readable':
	stream[kState].controller[kPull](readRequest);
	}
	}

If Undici was calling this directly, would that workaround the issue?

[KhafraDev]

probably not, I assume that's being used by .read already edit: yeah, that's in the stack trace

[aduh95]

It's being used by read indeed, but the issue you're having is that read returns Promise.resolve(anObjectThatInheritsFromObjectPrototype), which is what is accessing Object.prototype.then. Calling readableStreamDefaultReaderRead directly should workaround the issue (because you don't deal with promises anymore), or what am I missing?

[KhafraDev]

The error is coming from node:internal/webstreams/readablestream:771:27 (the stack trace is in the issue)

[aduh95]

The error is coming from node:internal/webstreams/readablestream:771:27 (the stack trace is in the issue)

That's calling Promise.resolve though:

node/lib/internal/webstreams/readablestream.js

Line 771 in d42628d

this[kState].resolve?.({ value, done: false });

[KhafraDev]

unless I'm not understanding something, readableStreamDefaultReaderRead eventually calls that resolve, which means exposing it won't fix our issue.

here is a better example of the code that's being tested (which browsers pass according to the WPT link):

import { Response } from 'undici'

const hello = new TextEncoder().encode('hello');
const bye = new TextEncoder().encode('bye');
const rs = new ReadableStream({
  start (controller) {
    controller.enqueue(hello);
    controller.close();
  }
});
const resp = new Response(rs);
Object.defineProperty(Object.prototype, 'then', {
  get () {
    throw new Error('..')
  }
})

const text = await resp.text();
delete Object.prototype.then;

[aduh95]

unless I'm not understanding something, readableStreamDefaultReaderRead eventually calls that resolve, which means exposing it won't fix our issue.

It's calling a method of the readRequest you pass as an argument, so my thinking is that you should be able to override the specific method that's causing an issue.