fs: allow FileHandle to outlive ReadableStream

[Slayer95]

Quoting @wolfgang42 at #45721 :

there's no way to only partially consume an fs.ReadStream created from a file handle without either closing the handle or leaking the stream.

[ronag]

I think #45909 is a better solution and doesn't involve weak refs.

[Slayer95]

As far as I can tell, #45909 fixes an event handler leak for streams that do get destroyed. In contrast, this PR fixes non-destroying iterators. So, in order not to leave loose ends in resource management. they are complementary rather than competing approaches.

[ronag]

Not destroying a stream is a bug though. So this is basically a way to make those bugs less severe/observable?

[Slayer95]

Not destroying a stream is a bug though

As per #38526, it looks like a feature to me. One may think that reusing a file handle after a stream reads from it should be supported, and that said stream should not be kept in memory. Am I missing something?

[ronag]

Streams should always be destroyed.

[ronag]

Destroying the stream just unrefs the handle. If there are other refs they are still usable.

[Slayer95]

Gotcha. Therefore, the issue of the file handle unexpectedly dying at #45721 arises because the file handle is left without refs to it. According to my reading, that's documented behavior, although somewhat controversial (is there currently a way to opt out of it in a per-handle basis?). Thanks for the pointers, @ronag .

Ok, so changing up the context. Suppose there is an HTTP server implemented using Node.js' FileHandle. A malicious agent requests some file very slowly to it. Simultaneously, there is normal traffic requesting the same file. If the server uses a single shared file handle for all those requests, then the close event of the file handle may be boundlessly delayed by the malicious request. Before it's emitted, all the requests streams will remain referred to by the file handle, eventually resulting in DoS. Unless... those streams can be GCed before the FileHandle emits close.

[wolfgang42]

In contrast, this PR fixes non-destroying iterators.

Streams should always be destroyed.

I think, unless I’m missing something, I do want to be destroying my streams (as @ronag says this is mandatory, which seems sensible); I just don’t want that to close the underlying file handle.

Destroying the stream just unrefs the handle. If there are other refs they are still usable.

As @Slayer95 noted in #45721 (comment), this doesn't seem to be true:

The docs also state that destroying the stream closes internal resources, which may only be understood as releasing the fd, and that's in fact all it does.

Therefore, the issue of the file handle unexpectedly dying at #45721 arises because the file handle is left without refs to it.

Unless I’ve misunderstood what you mean by either “handle” or “refs”, this is not true: in our examples, fh.fd gets printed after the handle is closed (resulting in -1), so we're obviously retaining a reference to fh to make that possible.

...malicious agent requests some file very slowly to it...

Worth noting that, if I’ve understood your scenario correctly, there doesn’t even need to be anything malicious for this to be a problem: I’m intending to keep this FileHandle open for potentially days at a time anyway, so I definitely need the streams to go away when I’m done with them. (I can’t just reopen the file by path because I’m relying on POSIX filesystem semantics for atomic updates, so someone might have replaced the file out from under me in the meantime and I still need the old data until I’m ready to swap.)

[Slayer95]

After testing the updated #45909 and a floating patch for proper access to [kFd], that approach looks sound and it fixes all concerns I have, so I'll close this PR in favor of yours, @ronag .