Skip to content

Conversation

@darai0512
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

doc

Description of change

About fs.read's 2nd argument, I think string is invalid.
https://github.com/nodejs/node/blob/v6.x-staging/src/node_file.cc#L1247-L1248

$cat fs_invalid_read_arg.js
const fs = require('fs');
fs.open('./fs_invalid_read_arg.js', 'r', (err, fd) => {
  const buffer = ''; // About 2nd argument, string is error. Buffer.alloc(1024) is ok.
  fs.read(fd, buffer, 0, 1024, 0, (err, byteRead, buffer) => {
    console.log('ok');
  })
});

$node fs_invalid_read_arg.js
fs.js:129
    throw new Error('Unknown encoding: ' + encoding);
    ^

Error: Unknown encoding: 1048576
    at assertEncoding (fs.js:129:11)
    at Object.fs.read (fs.js:654:5)
    at fs.open (/Users/daiki/workspace/fs_invalid_read_arg.js:5:6)
    at FSReqWrap.oncomplete (fs.js:123:15)

And also, about setImmediate, the timing of the execution is after timers in Event Loop of over v0.9, though it is correct in Event Loop under v0.8.
So, I deleted the description because the timing depended on Nodejs version.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 25, 2017
@vsemozhetbyt vsemozhetbyt added fs Issues and PRs related to the fs subsystem / file system. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Mar 25, 2017
@vsemozhetbyt
Copy link
Contributor

I may be wrong, but is it worth to be split into separate commits or even PRs?

@darai0512
Copy link
Contributor Author

@vsemozhetbyt
Sure, I will divide my commit into two commits.

About fs.read's 2nd argument, string is invalid.
About setImmediate, the execution timing is after timers currently.
@darai0512
Copy link
Contributor Author

@vsemozhetbyt
I divided it. PTAL.

@jasnell
Copy link
Member

jasnell commented Mar 27, 2017

@Fishrock... are you good with the Timers doc change here?

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.

^^^ That ping should probably have gone to @Fishrock123 ;)

jasnell pushed a commit that referenced this pull request Apr 4, 2017
About fs.read's 2nd argument, string is invalid.

PR-URL: #12034
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Apr 4, 2017
About setImmediate, the execution timing is after timers currently.

PR-URL: #12034
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

Landed in 0844262...e9f2ec4

@jasnell jasnell closed this Apr 4, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
About fs.read's 2nd argument, string is invalid.

PR-URL: nodejs#12034
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
About setImmediate, the execution timing is after timers currently.

PR-URL: nodejs#12034
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

is this accurate for v6.x?

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

is this accurate for v6.x?

ping

@darai0512
Copy link
Contributor Author

@MylesBorins @gibfahn
Sorry for my late reply.
I think so, these are accurate for v6.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants