Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

createReadStream/createWriteStream should support FileHandle #35240

Closed
ronag opened this issue Sep 17, 2020 · 4 comments
Closed

createReadStream/createWriteStream should support FileHandle #35240

ronag opened this issue Sep 17, 2020 · 4 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@ronag
Copy link
Member

ronag commented Sep 17, 2020

Currently only raw file descriptors are supported. Should we allow for a FileHandle to be passed as fd?
@ronag ronag added fs Issues and PRs related to the fs subsystem / file system. feature request Issues that request new features to be added to Node.js. labels Sep 17, 2020
@mmomtchev
Copy link
Contributor

Any advantages over simply using FileHandle.fd? 😃

@ronag
Copy link
Member Author

ronag commented Oct 13, 2020

Any advantages over simply using FileHandle.fd? 😃

Yes, closing the file handle would close the fd which would corrupt the stream, i.e. the reference counting is bypassed.

@mmomtchev
Copy link
Contributor

Suddenly this appears much more needed, I will take it of no one is already working on it?

@ronag
Copy link
Member Author

ronag commented Oct 30, 2020 •
edited

@mmomtchev go for it!

mmomtchev added a commit to mmomtchev/node that referenced this issue Dec 2, 2020
@mmomtchev
stream: add FileHandle support to Read/WriteStream
72873cf 
Support creating a Read/WriteStream from a
FileHandle instead of a raw file descriptor
Add an EventEmitter to FileHandle with a single
'close' event

Refs: nodejs#35240
mmomtchev added a commit to mmomtchev/node that referenced this issue Dec 2, 2020
@mmomtchev
stream: support FileHandle in Read/WriteStream
51640a5 
Add the PR URL to the documentation

Refs: nodejs#35922
Refs: nodejs#35240
mmomtchev added a commit to mmomtchev/node that referenced this issue Dec 2, 2020
@mmomtchev
stream: support FileHandle for Read/WriteStream
7713743 
Use primordials where necessary (@aduh95) and
check that the data was actually written in the
unit test

Refs: nodejs#35922
Refs: nodejs#35240
@aduh95 aduh95 closed this as completed in 0fd121e Dec 4, 2020
danielleadams pushed a commit that referenced this issue Dec 7, 2020
@danielleadams
stream: add FileHandle support to Read/WriteStream
6033d30 
Support creating a Read/WriteStream from a
FileHandle instead of a raw file descriptor
Add an EventEmitter to FileHandle with a single
'close' event.

Fixes: #35240

PR-URL: #35922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this issue Dec 8, 2020
@mmomtchev @cjihrig
stream: add FileHandle support to Read/WriteStream
424858b 
Support creating a Read/WriteStream from a
FileHandle instead of a raw file descriptor
Add an EventEmitter to FileHandle with a single
'close' event.

Fixes: nodejs#35240

PR-URL: nodejs#35922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ronag @mmomtchev