Skip to content

stream: add FileHandle support to Read/WriteStream#35922

Closed
mmomtchev wants to merge 20 commits intonodejs:masterfrom
mmomtchev:stream-filehandle
Closed

stream: add FileHandle support to Read/WriteStream#35922
mmomtchev wants to merge 20 commits intonodejs:masterfrom
mmomtchev:stream-filehandle

Conversation

@mmomtchev
Copy link
Contributor

@mmomtchev mmomtchev commented Nov 2, 2020

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: #35240

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 2, 2020
richardlau
richardlau reviewed Nov 2, 2020
View reviewed changes
@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 2, 2020
aduh95
aduh95 reviewed Nov 2, 2020
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few suggestions to use primordials when possible, PTAL.

addaleax
addaleax reviewed Nov 2, 2020
View reviewed changes
aduh95
aduh95 reviewed Nov 2, 2020
View reviewed changes
@benjamingr
Copy link
Member

benjamingr commented Nov 2, 2020

Our file handles are pretty huge anyway and I am not thrilled about adding all those extra properties to them, I don't really understand the use case :/

I'm -0 but won't block

(good job on the actual changes, the code itself looks good 👍 ).

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Nov 2, 2020

Our file handles are pretty huge anyway and I am not thrilled about adding all those extra properties to them, I don't really understand the use case :/

I'm -0 but won't block

(good job on the actual changes, the code itself looks good ).

#35862 - this is the use case

aduh95
aduh95 reviewed Nov 2, 2020
View reviewed changes
addaleax
addaleax reviewed Nov 2, 2020
View reviewed changes
addaleax
addaleax approved these changes Nov 2, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work :)

aduh95
aduh95 reviewed Nov 2, 2020
View reviewed changes
Trott
Trott reviewed Nov 13, 2020
View reviewed changes
@Trott
Copy link
Member

Trott commented Nov 13, 2020

(Needs a rebase.)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 2, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 2, 2020

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

@aduh95
Copy link
Contributor

aduh95 commented Dec 4, 2020

Landed in 0fd121e

@aduh95 aduh95 closed this Dec 4, 2020
aduh95 pushed a commit that referenced this pull request Dec 4, 2020
@mmomtchev @aduh95
stream: add FileHandle support to Read/WriteStream
0fd121e 
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>
@ronag

This comment has been minimized.

Sign in to view
@mmomtchev
Copy link
Contributor Author

mmomtchev commented Dec 6, 2020

@ronag I don't see it? What is the function sequence?

@ronag
Copy link
Member

ronag commented Dec 6, 2020

@mmomtchev you are right. There is no problem here.

@aduh95 aduh95 mentioned this pull request Dec 6, 2020
Closed
2 tasks
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
@mmomtchev @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>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
Merged
danielleadams added a commit that referenced this pull request Dec 7, 2020
@danielleadams
2020-12-08, Version 15.4.0 (Current)
fde09a4 
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
cjihrig pushed a commit to cjihrig/node that referenced this pull request 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>
danielleadams added a commit that referenced this pull request Dec 9, 2020
@danielleadams
2020-12-09, Version 15.4.0 (Current)
5e5927e 
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
@danielleadams
2020-12-09, Version 15.4.0 (Current)
348b7a9 
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@richardlau richardlau richardlau left review comments

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@mmomtchev @benjamingr @Trott @nodejs-github-bot @aduh95 @addaleax @ronag @richardlau @targos