doc: add and fix System Error properties by darai0512 · Pull Request #10986 · nodejs/node

darai0512 · 2017-01-25T00:32:12Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

In System Errors properties of Errors API, error.path, error.address and error.port are not described although being also represented as augmented Error objects with added properties by the following code.

properties	link
address, port	https://github.com/nodejs/node/blob/v6.x-staging/lib/util.js#L1030-L1051
path	https://github.com/nodejs/node/blob/master/lib/child_process.js#L526

// examples
// with `path` property
$node -e "require('fs').readFile('a file that does not exist', (err, data) => { console.error(err); });"
{ Error: ENOENT: no such file or directory, open 'a file that does not exist'
    at Error (native)
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: 'a file that does not exist' }

// with `address` and `port` property
$node -e "require('net').connect({port: 100}).on('error', (err) => { console.error(err); });"
{ Error: connect ECONNREFUSED 127.0.0.1:100
    at Object.exports._errnoException (util.js:1022:11)
    at exports._exceptionWithHostPort (util.js:1045:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1087:14)
  code: 'ECONNREFUSED',
  errno: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 100 }

// with `address` property (without `port` property)
$node -e "require('net').connect({host: 'localhost'}).on('error', (err) => { console.error(err); });"
{ Error: connect EADDRNOTAVAIL 127.0.0.1 - Local (0.0.0.0:54881)
    at Object.exports._errnoException (util.js:1022:11)
    at exports._exceptionWithHostPort (util.js:1045:20)
    at connect (net.js:881:16)
    at net.js:1010:7
    at GetAddrInfoReqWrap.asyncCallback [as callback] (dns.js:62:16)
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:81:10)
  code: 'EADDRNOTAVAIL',
  errno: 'EADDRNOTAVAIL',
  syscall: 'connect',
  address: '127.0.0.1' }

Also, error.errno doesn't always return a number like the above examples.
Please check the following code.

https://github.com/nodejs/node/blob/v6.x-staging/lib/util.js#L1024

mscdex · 2017-01-25T01:46:10Z

doc/api/errors.md

s/mudules/modules/

@mscdex Thanks for your review.

mscdex · 2017-01-25T01:47:58Z

doc/api/errors.md

Despite the term 'returns' being better suited for functions rather than properties, this would still be better wording IMHO: 'Returns a string containing the relevant invalid pathname (where applicable).'

Similarly for the other properties added below.

Even though there are existing contrary examples in this very file, I think we should avoid saying returns when talking about a property. We should reserve returns for functions.

So, instead of Returns a string that..., just A string that.... Yes, it will technically be a sentence fragment, but I think it's acceptable in this case. It would certainly be clear and understandable which is the most important thing.

/cc @nodejs/documentation

I feel like a simple s/returns/is/ would make sense, in most cases.

@mscdex @Trott @Qard Thanks for your review.
I did s/Returns/Is/ about this page's properties.

Hate to disagree with @Qard, but -1 on this approach. We don't start descriptions with Is anywhere else in the docs. Let's not create a third way to do things. Let's standardize on one of the existing approaches.

IMO, this should be: A string of the specified invalid pathname in case of using some modules... rather than Is a string of the specified...

I'm good either way. Just figured "is" might provide a better hint as to the access method.

darai0512 · 2017-01-25T14:56:27Z

About the following expression as well, not 'returns' but 'is' may be better.
https://github.com/nodejs/node/blame/master/doc/api/errors.md#L208
But I think it is natural enough.

mscdex · 2017-01-25T14:56:43Z

doc/api/errors.md

'Same string?' Above it is mentioned that this is a number, not a string.

mscdex · 2017-01-25T15:02:37Z

doc/api/errors.md

'the specified' here is confusing. It makes it sound like it's a value the user is passing into a function or something. As previously suggested, I would use something like:

Is a string containing a relevant invalid path name (where applicable). Some modules that include this property are fs and child_process.

and structuring the other two property descriptions similarly.

@mscdex
I understood. I will fix.

sam-github · 2017-01-25T16:23:04Z

doc/api/errors.md

I know, its miserable trying to figure exactly which modules do this, but this doc reads as "behaviour is random, shrug". Consider giving some advice on what to do:

const errno = error.errno || error.code;

That may not be preferrable if errno is a number, whereas code is a string? You'd have to explicitly cast to a string.

OK, then the docs should say "In some modules, is a number corresponding to ...etc." because the first sentence is being contradicted by the paragraph after ATM.

Is there an open issue to track this? It doesn't strike me as so helpful to sometimes be an "E" string, and sometimes a number.

I'm not familiar enough with the libuv error codes, but I would guess that the .code would be the same across platforms whereas the .errno may not be?

I heard this from @darai0512 a few days ago and also was surprised to know that.
I think we cannot change them for there are codes expecting errno is string.

I think it is better to fix like this. How is it?

--- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -455,9 +455,11 @@ a sequence of capital letters, and may be referenced in `man 2 intro`. #### error.errno -Returns a number corresponding to the **negated** error code, which may be -referenced in `man 2 intro`. For example, an `ENOENT` error has an `errno` of -`-2` because the error code for `ENOENT` is `2`. +Returns a number or a string. The number corresponds to the +**negated** error code, which may be referenced in `man 2 intro`. For +example, an `ENOENT` error has an `errno` of `-2` because the error +code for `ENOENT` is `2`. +In case of a string, it is the same as error.code

I will modify it according to @shigeki advise.

sam-github · 2017-01-25T16:45:05Z

doc/api/errors.md

"When present, "....

Is it always an address that is not available? Can't it show up in errors for other reasons?

This appears only in the error that comes from net and dgram. I think that is why it was missed in the doc. I told him to add what modules it happens in. How about this?

When present (e.g. in net and dgram), it is a string describing the address that is not available.

OK, I will fix.

I like@shigeki's text, but I'm not sure what "not available means". It means that the address cannot be looked up?

@sam-github
Exactly, so I rewrite the description. Please review.

sam-github · 2017-01-25T16:45:50Z

doc/api/errors.md

Doesn't it show for other reasons, such as connection reset?

"When present, ..."

Doesn't it show for other reasons, such as connection reset?

Exactly. The same description of error.address as is not available seems to be better.

sam-github · 2017-01-25T16:46:56Z

doc/api/errors.md

"When present, ..."

@sam-github
Excuse me, is there some cases without error.syscall property?

jasnell

Almost there. It may also be worth documenting the error.name property.

jasnell · 2017-01-26T15:14:40Z

doc/api/errors.md

I would prefer something like:

The `error.message` property is the...

@jasnell Thanks for your review. I will fix.

jasnell · 2017-01-26T15:16:15Z

doc/api/errors.md

We should be clear here. I know this was already in the doc, but may not is not that clear. We should say whether it will or will not or indicate the conditions under which it may or may not.

I also got confused about its expression.
I know a simple example such as the following code.

Error.stackTraceLimit = 0; var error = new Error('This will not change'); console.log(error.stack); // Prints: Error: This will not change error.message = 'not changed'; console.log(error.stack); // Prints: Error: This will not change

Error.stackTraceLimit = 0; var error = new Error('This will change'); error.message = 'changed'; console.log(error.stack); // Prints: Error: changed

So, I will add the statement of (for example, when error.stack is set before this property is changed)

darai0512 · 2017-01-29T11:30:56Z

@jasnell

It may also be worth documenting the error.name property.

I think error.name belongs to ecma as Error.prototype.name originally.
https://www.ecma-international.org/ecma-262/7.0/#sec-error-objects

So, I think there is no necessity to explain it here. How about?

sam-github

other than that the man page ref seems wrong on linux, LGTM

sam-github · 2017-01-31T21:55:19Z

doc/api/errors.md

I don't see them refed on linux in the syscall intro, I do see them in man 3 errno.

@sam-github
Exactly. The command is already explained. So, I think there is no necessity to explain it here and in error.errno.
https://github.com/darai0512/node/blame/85341c7ae573c31c43ce05549e8f789138e047fd/doc/api/errors.md#L445-L446

sam-github · 2017-01-31T21:57:48Z

This is close to ready, can you squash and rebase please?

mscdex · 2017-02-01T15:16:05Z

doc/api/errors.md

'error.stack' here should have backticks around it.

OK, I will fix: error.stack -> error.stack

mscdex · 2017-02-01T15:17:23Z

doc/api/errors.md

'error.code' here should be surrounded by backticks.

OK, I will fix: error.code -> error.code

mscdex · 2017-02-01T15:18:34Z

doc/api/errors.md

Might want to double-check the applicability of this information to Windows systems ...

I fix about the command. I think it is already explained in L445-446, so there is no necessity to explain it again.

running man 2 intro or man 3 errno on most Unices; or [online][].

But exactly, it may not be referred to Windows systems.
I think 'online' is covered about it. How about?

I wasn't referring to the man reference, but instead the information about the error code being negated and such (e.g. the number may be positive on Windows from the start? is the number from the man pages still relevant for Windows -- does libuv normalize them on Windows?).

Sorry, I understood your point. The errno of this example may not be -2 on Windows. I will look over......

@mscdex Good catch. Right, the Windows error is mapped to libuv error in https://github.com/nodejs/node/blob/master/deps/uv/src/win/error.c#L66 then allocated to an arbitrary negative number in uv-errno.h. I think that it is better not to describe this further here but just put reference to libuv.

diff --git a/doc/api/errors.md b/doc/api/errors.md index 86d06fa..9a72cc9 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -458,9 +458,10 @@ The `error.code` property is a string representing the error code, which is alwa #### error.errno The `error.errno` property is a number or a string. -The number corresponds to the **negated** error code. For example, an `ENOENT` -error has an `errno` of `-2` because the error code for `ENOENT` is `2`. -In case of a string, it is the same as `error.code`. +The number corresponds to the **negated** error code which is defined +in [`libuv Error handling`]. See uv-errno.h header file +(`deps/uv/include/uv-errno.h` in the Node.js source tree) for +details. In case of a string, it is the same as `error.code`. #### error.syscall @@ -546,6 +547,7 @@ found [here][online]. [`fs`]: fs.html [`http`]: http.html [`https`]: https.html +[`libuv Error handling`]: http://docs.libuv.org/en/v1.x/errors.html [`net`]: net.html [`process.on('uncaughtException')`]: process.html#process_event_uncaughtexception [domains]: domain.html

@mscdex PTAL

@darai0512 It still shows the old text for this property?

@mscdex Really? I corrected the text of error.errno along @shigeki 's proposal.

@darai0512 My last proposal is

The `error.errno` property is a number or a string. The number is a **negative** value which corresponds to the error code defined in [`libuv Error handling`]. See uv-errno.h header file (`deps/uv/include/uv-errno.h` in the Node.js source tree) for details. In case of a string, it is the same as `error.code`.

The current your commit does not have it.

@mscdex @shigeki
Sorry, not 'the negated error code' but 'a negative value' because uv errno is used.
PTAL

darai0512 · 2017-02-01T16:46:08Z

@sam-github
I squash and rebase. Please check.

sam-github · 2017-02-01T17:53:34Z

@mscdex had comments and needs to review

TimothyGu

While you are at it, please also add type annotations (like the ones in http)

TimothyGu · 2017-02-02T03:52:25Z

doc/api/errors.md

There should be either a paragraph break, or the lines should be wrapped together. It's not immediately clear what this is trying to do.

Okay, I will change it to 1 line.

TimothyGu · 2017-02-02T03:53:16Z

doc/api/errors.md

Can you elaborate on when error.stack is "set"?

I thought "called" was good at first. But it is good for not property but function.
"read" is good?

TimothyGu · 2017-02-02T03:56:22Z

doc/api/errors.md

Is ENOENT guaranteed to be 2 on all platforms by a specification? Or is it true de facto?

No, it may vary either on unix or on win32. @mscdex also refers to it. I will check it...

TimothyGu · 2017-02-02T03:59:43Z

doc/api/errors.md

"port that is not available" sounds too specific to me. Is port binding the only case in Node.js when err.port is present? Either way, I'd prefer "a relevant port number", which is intentionally vague but more future-proof.

Also, "... property is a number representing ..." sounds slightly better.

Thanks for your review. I will fix the following text.

=> the error.port property is a number representing the connection's port that is not available.

darai0512 · 2017-02-03T05:24:05Z

@TimothyGu PTAL

mscdex · 2017-02-03T07:14:32Z

doc/api/errors.md

s/of error/of the error/ or add backticks around 'error'

darai0512 · 2017-02-03T15:58:34Z

PTAL

darai0512 · 2017-02-07T06:53:55Z

Sorry, I made a mistake about @shigeki 's proposal. So, I fixed it.
PTAL

About path, address and port properties, these are not described though being also represented as augmented Error objects with added properties. And also, fix all property descriptions and add type annotations.

About path, address and port properties, these are not described though being also represented as augmented Error objects with added properties. And also, fix all property descriptions and add type annotations. PR-URL: #10986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

jasnell · 2017-02-08T00:09:50Z

Landed in fcedd71

About path, address and port properties, these are not described though being also represented as augmented Error objects with added properties. And also, fix all property descriptions and add type annotations. PR-URL: nodejs#10986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

About path, address and port properties, these are not described though being also represented as augmented Error objects with added properties. And also, fix all property descriptions and add type annotations. PR-URL: #10986 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. lts-watch-v6.x labels Jan 25, 2017

mscdex reviewed Jan 25, 2017

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Jan 25, 2017

mscdex reviewed Jan 25, 2017

View reviewed changes

doc/api/errors.md Outdated

Copy link

Contributor

mscdex Jan 25, 2017

Choose a reason for hiding this comment

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

'Same string?' Above it is mentioned that this is a number, not a string.

mscdex reviewed Jan 25, 2017

View reviewed changes

sam-github reviewed Jan 25, 2017

View reviewed changes

sam-github approved these changes Jan 25, 2017

View reviewed changes

sam-github suggested changes Jan 25, 2017

View reviewed changes

jasnell requested changes Jan 26, 2017

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Jan 29, 2017

sam-github suggested changes Jan 31, 2017

View reviewed changes

mscdex reviewed Feb 1, 2017

View reviewed changes

darai0512 force-pushed the doc/errors branch from 85341c7 to 3bb285b Compare February 1, 2017 16:37

nodejs-github-bot removed lts-watch-v6.x labels Feb 1, 2017

sam-github approved these changes Feb 1, 2017

View reviewed changes

sam-github requested a review from mscdex February 1, 2017 17:52

TimothyGu reviewed Feb 2, 2017

View reviewed changes

darai0512 force-pushed the doc/errors branch from 3bb285b to 435a62e Compare February 2, 2017 18:01

mscdex reviewed Feb 3, 2017

View reviewed changes

doc/api/errors.md Outdated

Copy link

Contributor

mscdex Feb 3, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

s/of error/of the error/ or add backticks around 'error'

darai0512 force-pushed the doc/errors branch from 435a62e to 28407ae Compare February 3, 2017 15:58

darai0512 force-pushed the doc/errors branch 2 times, most recently from 05d3541 to baaf148 Compare February 7, 2017 06:49

doc: add and fix System Error properties

d2319e2

About path, address and port properties, these are not described though being also represented as augmented Error objects with added properties. And also, fix all property descriptions and add type annotations.

darai0512 force-pushed the doc/errors branch from baaf148 to d2319e2 Compare February 7, 2017 07:07

jasnell approved these changes Feb 8, 2017

View reviewed changes

jasnell closed this Feb 8, 2017

italoacasas mentioned this pull request Feb 13, 2017

Proposal - 7.6.0 (Current) #11185

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

darai0512 deleted the doc/errors branch April 5, 2017 03:53

Uh oh!

Conversation

darai0512 commented Jan 25, 2017

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darai0512 commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 25, 2017 •

edited

Loading

Trott Jan 25, 2017 •

edited

Loading

mscdex Jan 25, 2017 •

edited

Loading

darai0512 Jan 27, 2017 •

edited

Loading

darai0512 commented Jan 29, 2017 •

edited

Loading