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

doc: fs.readFile is async but not partitioned #17154

Closed
wants to merge 1 commit into from
Closed

doc: fs.readFile is async but not partitioned #17154

wants to merge 1 commit into from

Conversation

davisjam
Copy link
Contributor

This change was suggested during the discussion of #17054.

Affected core subsystem(s)

docs, fs

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Nov 20, 2017
jasnell
jasnell reviewed Nov 20, 2017
View reviewed changes
doc/api/fs.md Outdated
@@ -1897,6 +1897,10 @@ Any specified file descriptor has to support reading.
*Note*: If a file descriptor is specified as the `path`, it will not be closed
automatically.

*Note*: `fs.readFile()` reads the entire file in a single threadpool request.
To minimize threadpool task variation, prefer the partitioned APIs `fs.read()`
and `ReadStream` when reading files as part of fulfilling a client request.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: using fs.createReadStream() instead of ReadStream here may be clearer for most users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6789de6

@joyeecheung
Copy link
Member

linter CI: https://ci.nodejs.org/job/node-test-linter/13776/

@joyeecheung
Copy link
Member

Pure doc change, multiple approvals, I think this one can be fast-tracked.

@joyeecheung joyeecheung added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 21, 2017
@davisjam davisjam mentioned this pull request Nov 21, 2017
Closed
3 tasks
davisjam added a commit to davisjam/node that referenced this pull request Nov 22, 2017
@davisjam
doc: non-partitioned async crypto operations
158692e 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of nodejs#17054.
See also nodejs#17154.
@davisjam davisjam mentioned this pull request Nov 22, 2017
Closed
2 tasks
@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

@davisjam ... can I ask you to please squash the two commits.

@davisjam
doc: fs.readFile is async but not partitioned
f32cbac 
This change was suggested during the discussion of nodejs#17054.
@davisjam
Copy link
Contributor Author

@jasnell Fixed

jasnell pushed a commit that referenced this pull request Nov 22, 2017
@davisjam @jasnell
doc: fs.readFile is async but not partitioned
a59ad82 
This change was suggested during the discussion of #17054.

PR-URL: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

Landed in a59ad82

@jasnell jasnell closed this Nov 22, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
addaleax pushed a commit that referenced this pull request Nov 28, 2017
@davisjam @addaleax
doc: non-partitioned async crypto operations
16e87ed 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of #17054.
See also #17154.

PR-URL: #17250
Refs: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davisjam @MylesBorins
doc: fs.readFile is async but not partitioned
dd918ae 
This change was suggested during the discussion of #17054.

PR-URL: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davisjam @MylesBorins
doc: non-partitioned async crypto operations
d500f04 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of #17054.
See also #17154.

PR-URL: #17250
Refs: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davisjam @MylesBorins
doc: non-partitioned async crypto operations
6529076 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of #17054.
See also #17154.

PR-URL: #17250
Refs: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davisjam @MylesBorins
doc: fs.readFile is async but not partitioned
04c4c1f 
This change was suggested during the discussion of #17054.

PR-URL: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@davisjam @MylesBorins
doc: non-partitioned async crypto operations
0488782 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of #17054.
See also #17154.

PR-URL: #17250
Refs: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@davisjam @gibfahn
doc: fs.readFile is async but not partitioned
ed9296d 
This change was suggested during the discussion of #17054.

PR-URL: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@davisjam @gibfahn
doc: non-partitioned async crypto operations
a2c82c3 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of #17054.
See also #17154.

PR-URL: #17250
Refs: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@davisjam @gibfahn
doc: fs.readFile is async but not partitioned
ec1a35c 
This change was suggested during the discussion of #17054.

PR-URL: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@davisjam @gibfahn
doc: non-partitioned async crypto operations
5d1b76c 
Neither crypto.randomBytes nor crypto.randomFill
partitions the work submitted to the threadpool.

This change was suggested during the discussion of #17054.
See also #17154.

PR-URL: #17250
Refs: #17154
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@davisjam @joyeecheung @jasnell @Trott @gireeshpunathil @vsemozhetbyt @MylesBorins @addaleax @gibfahn @nodejs-github-bot