streams: fixes for webstreams

[MattiasBuelens]

Follow-up on #50126.

• Avoid PromiseResolve in ensureIsPromise. Web IDL requires this to always return a new Promise, whereas PromiseResolve may return the same Promise. This leads to a different timing, since new Promise() takes at least one microtask to resolve. See my previous comment.
• Fix handling sync errors from source cancel and sink abort. We were not correctly handling synchronously-thrown errors from these, which could leave the stream in an incorrect state.
• Check for promise only when constructing from source/sink/transformer. When constructing an internal stream, these algorithms must always return a promise, and we shouldn't wrap them again with ensureIsPromise().
• Fix TeeReadableStream. This should construct an internal stream with internal callbacks using setupReadableStreamDefaultController, but it was incorrectly using user-land callbacks with setupReadableStreamDefaultControllerFromSource.
• Use internal streams for teeing a readable byte stream. This was using the user-land ReadableStream constructor, which is incorrect. The new implementation mirrors how we handle teeing "default" readable streams.
• Use internal streams for ReadableStream.from(). Again, this was incorrectly using the user-land ReadableStream constructor.
• Use internal streams for TransformStream. Same thing again.
• Add a helper to create the internal state of a ReadableStream and WritableStream. This code was duplicated a few times, so it made sense to extract a helper function.
• Remove an unused field in the ReadableStream internal state. This seemed like a copy-paste mistake: other classes (like ReadableStreamDefaultController) do have a stream field in their state, but ReadableStream shouldn't.
• Fix validating callbacks. Previously, we asserted that the given callbacks were indeed typeof callback === "function" by virtue of bluntly trying to call FunctionPrototypeBind on them. We no longer use FunctionPrototypeBind for this, so we need to manually call validateFunction for each callback. I've wrapped that in a createPromiseCallback helper.
• Update expectations for the Streams WPT tests.

[nodejs-github-bot]

Review requested:

• @nodejs/web-standards

[ljharb]

if it's always making a new promise, any reason not to do this?

Suggested change

	function ensureIsPromise(fn, thisArg, ...args) {
	try {
	const value = FunctionPrototypeCall(fn, thisArg, ...args);
	return isPromise(value) ? value : PromiseResolve(value);
	return new Promise((r) => r(value));
	} catch (error) {
	return PromiseReject(error);
	}
	}
	async function ensureIsPromise(fn, thisArg, ...args) {
	return await FunctionPrototypeCall(fn, thisArg, ...args);
	}

[MattiasBuelens]

I like that a lot! An async function always returns a fresh Promise, so this should satisfy Web IDL. 🙂

[MattiasBuelens]

Unfortunately, this change causes some tests to fail again... 😞

However, if you remove the await, it does work:

async function ensureIsPromise(fn, thisArg, ...args) {
  return FunctionPrototypeCall(fn, thisArg, ...args);
}

You still get a fresh Promise (from async function), errors are still correctly turned into rejections, and the timings match Web IDL.

Still, I don't like how sensitive these tests are to the number of microtasks. I'll try to fix this in upstream WPT.

[debadree25]

cc @nodejs/whatwg-stream

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56336/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56465/

[nodejs-github-bot]

Landed in 20c6313

[marco-ippolito]

this doesn't land cleanly on v20 requires a manual backport

[MattiasBuelens]

@marco-ippolito This PR builds upon #50107, specifically: 0d45e6f needs a85e418. However, that other PR is marked "dont-land-on-v20.x". 😕

I'm not sure how to proceed with a manual backport. Should I try to rework my changes without that PR, or should we backport #50107 to v20.x anyway?

[marco-ippolito]

I guess then we wont backport it, @targos I saw you marked #50107 as dont not land on v20, wdyt

[MattiasBuelens]

On second thought, I think I can rework my changes. Let me try something... 👨‍🔬

[MattiasBuelens]

@marco-ippolito I managed to make it work without #50107, see #52773.

However, If we also want to backport #50107, then I'll need to change it up again. 😛 Let me know how you want to proceed.

[targos]

I marked it dont-land because it depended on #47956, which was dont-land. Feel free to remove the label and backport.