fs: add FileHandle.prototype.readableStream()

[jasnell]

Builds on #39134 (the first commit is from that PR)

Adds an experimental readableStream() method to FileHandle objects that returns ReadableStream.

const file = await fs.promises.open('./some/file/to/read');

for await (const chunk of file.readableStream())
  console.log(chunk);

const file = await fs.promises.open('./some/file/to/read');

const readable = file.readableStream();
const reader = readable.getReader();
reader.read().then(console.log);

Note that the ReadableStream returned is affected by the other methods for reading the FileHandle data. For instance, if file.readFile() is called first, the ReadableStream will not return any data.

[ronag]

readableWebStream? Also what is the advantage of this relative to just making it async iterable?

[ronag]

I would prefer an API with less surface area e.g.:

await dstHandle.write(srcHandle)

Where write takes an async iterable and srcHandle is an async iterable.

Later could turn into:

await dstHandle.write(pipeline(nodeReadable, webTransform, nodeTransform, () => {}))
await dstHandle.write(compose(nodeReadable, webTransform, nodeTransform))

[jasnell]

Less surface area? I don't understand. It's a single function with no arguments and is consistent with common web platform APIs. What you're suggesting is a different pattern that can also be implemented but that shouldn't block this.

[ronag]

Less surface area? I don't understand. It's a single function with no arguments and is consistent with common web platform APIs. What you're suggesting is a different pattern that can also be implemented but that shouldn't block this.

I'm just concerned that we start hooking up this to various things in node and end up with 3+ different ways to consume everything. I would prefer to have as few as possible (1?) so that there is less confusion about when to use what.

Also I'm unsure about the naming here again. It's ambiguous whether this is a node readable or a web readable.

We already have 5 different ways to consume node streams which leads to a lot of confusion in regards to when to use what. Would like to try and avoid a repeat of that.

Just concerned. Not blocking.

[ronag]

Btw, https://developer.mozilla.org/en-US/docs/Web/API/Blob/stream web File uses .stream()?

[jasnell]

@ronag:

Btw, developer.mozilla.org/en-US/docs/Web/API/Blob/stream web File uses .stream()?

Yep, and Response (part of fetch) uses .body. Some of the other web apis use .readable, it's a mess.

readableWebStream() is ok here but isn't great.

[jasnell]

@ronag - updated to use readableWebStream()

[nodejs-github-bot]

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

[mcollina]

lgtm

[ronag]

Needs either a ref/unref or a on('close', () => stream.close()). I think in node streams we do the latter.

[jasnell]

@ronag ... I added the ref/unref with additional tests, and a note in the docs that the FileHandle must still be closed by the usercode. PTAL

[mcollina]

lgtm

[jasnell]

@addaleax ... I'd appreciate if you could take a look at the third commit here. It changes FileHandle::ClosePromise to always return the same persisted promise after the first call as opposed to throwing if Close is called again.

[jasnell]

Landed in e2a6399...e239f5e

[targos]

Depends on #39134

[Trott]

This test fails on containered tests a lot it seems? Here's a link to the most recent daily-master where it failed on three containers. https://ci.nodejs.org/job/node-test-commit-linux-containered/27906/

[jasnell]

That's odd. Let's Mark it flaky for now and I'll investigate on Monday