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

stream: add FileHandle support to Read/WriteStream #35922

Closed
wants to merge 20 commits into from
Closed

stream: add FileHandle support to Read/WriteStream #35922

wants to merge 20 commits into from

Conversation

mmomtchev
Copy link
Contributor

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
doc/api/fs.md Outdated Show resolved Hide resolved
@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.

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Nov 2, 2020
View reviewed changes
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

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

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
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
addaleax
addaleax reviewed Nov 2, 2020
View reviewed changes
lib/events.js Outdated Show resolved Hide resolved
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
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 :)

test/parallel/test-fs-write-stream-file-handle.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Nov 2, 2020
View reviewed changes
lib/internal/event_target.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
Trott
Trott reviewed Nov 13, 2020
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
@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

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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ronag

This comment has been minimized.

Sign in to view
@mmomtchev
Copy link
Contributor Author

@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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

@aduh95 aduh95 aduh95 left review comments

@richardlau richardlau richardlau left review comments

@addaleax addaleax addaleax approved these changes

@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