fs: allow FileHandle to outlive ReadableStream#45917
fs: allow FileHandle to outlive ReadableStream#45917Slayer95 wants to merge 3 commits intonodejs:mainfrom
Conversation
|
I think #45909 is a better solution and doesn't involve weak refs. |
|
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. |
|
Not destroying a stream is a bug though. So this is basically a way to make those bugs less severe/observable? |
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? |
|
Streams should always be destroyed. |
|
Destroying the stream just unrefs the handle. If there are other refs they are still usable. |
|
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' |
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.
As @Slayer95 noted in #45721 (comment), this doesn't seem to be true:
Unless I’ve misunderstood what you mean by either “handle” or “refs”, this is not true: in our examples,
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 |
Quoting @wolfgang42 at #45721 :