doc: note caveats in process message serialization #12963

joyeecheung · 2017-05-11T05:40:52Z

The message sent using process.send() goes through JSON
serialization and parsing, which could lead to surprising behaviors.
This commit elaborate a bit more on this and add a link to
the notes about these caveats in the ECMAScript specification.

Refs: #12497

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, process, child_process

mscdex · 2017-05-11T05:55:38Z

doc/api/child_process.md

@@ -849,6 +849,10 @@ added: v0.5.9
 The `'message'` event is triggered when a child process uses [`process.send()`][]
 to send messages.

+*Note*: since the message goes through JSON serialization and parsing, it might
+not be the same as what is originally sent, see notes of
+[the `JSON.stringify()` specification][`JSON.stringify` spec].


Perhaps simplify the link and adjust some of the wording here... for example:

*Note*: since the message is transmitted as JSON, not all values may be transferred as-is. See the documentation for [`JSON.stringify()`][] for more details.

Where the JSON.stringify() link goes to MDN instead (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Description), which IMHO has a more readable description of when/how values are converted.

The MDN documentation doesn't mention the Nan/Infinity -> null thing which I think is one of the surprising behaviors..the link is not about the serialization process, but rather, the caveats, which are described in the notes of the ECMA spec (although I cannot find a direct anchor to those notes, so I use the link to the method instead).

Also the MDN documentation doesn't mention the circular reference error...:/ hmm

Nit: The comma after sent should be a period. See notes at... can be its own sentence.

mscdex · 2017-05-11T05:56:39Z

doc/api/child_process.md

@@ -996,6 +1000,13 @@ i.e. when using [`child_process.fork()`][]), the `child.send()` method can be
 used to send messages to the child process. When the child process is a Node.js
 instance, these messages can be received via the [`process.on('message')`][] event.

+*Note*: this function uses [`JSON.stringify()`][] internally to serialize the


I would just use the exact same text as suggested above for this part.

Nit: this -> This

mscdex · 2017-05-11T05:57:21Z

doc/api/process.md

 * `sendHandle` {Handle object} a [`net.Socket`][] or [`net.Server`][] object, or
  undefined.

+*Note*: since the message goes through JSON serialization and parsing, it might


Nit: since -> Since

mscdex · 2017-05-11T05:57:30Z

doc/api/process.md

@@ -1429,8 +1432,12 @@ used to send messages to the parent process. Messages will be received as a
 If Node.js was not spawned with an IPC channel, `process.send()` will be
 `undefined`.

-*Note*: This function uses [`JSON.stringify()`][] internally to serialize the
-`message`.*
+*Note*: this function uses [`JSON.stringify()`][] internally to serialize the


Nit: this -> This

Trott · 2017-05-11T18:30:12Z

doc/api/process.md

 * `sendHandle` {Handle object} a [`net.Socket`][] or [`net.Server`][] object, or
  undefined.

+*Note*: since the message goes through JSON serialization and parsing, it might
+not be the same as what is originally sent, see notes of


Identical nit as above about comma and see notes.

cjihrig

I think the existing messaging (Note: this function uses [JSON.stringify()][] internally to serialize the message.) is good. The message should probably be moved to a better location in the send() docs though.

I don't think we should explain too much about how stringify() works, when we are linking to it already.

cjihrig · 2017-05-11T19:56:41Z

doc/api/child_process.md

@@ -1258,6 +1268,7 @@ to `stdout` although there are only 4 characters.
 [`child_process.fork()`]: #child_process_child_process_fork_modulepath_args_options
 [`child_process.spawn()`]: #child_process_child_process_spawn_command_args_options
 [`child_process.spawnSync()`]: #child_process_child_process_spawnsync_command_args_options
+[`JSON.stringify` spec]: https://tc39.github.io/ecma262/#sec-json.stringify


JSON.stringify() is already in this list of links in both of these files.

sam-github · 2017-05-11T20:20:51Z

doc/api/process.md

@@ -1836,6 +1843,7 @@ cases:
 [`console.error()`]: console.html#console_console_error_data_args
 [`console.log()`]: console.html#console_console_log_data_args
 [`end()`]: stream.html#stream_writable_end_chunk_encoding_callback
+[`JSON.stringify` spec]: https://tc39.github.io/ecma262/#sec-json.stringify


please keep the refs sorted

BridgeAR · 2017-08-26T23:46:18Z

Ping @joyeecheung this needs a rebase and the comments still need to be addressed

Trott · 2017-08-27T02:06:37Z

Ping @joyeecheung this needs a rebase and the comments still need to be addressed

@joyeecheung I'm happy to push a commit for fixes for my nits if you'd like. Just let me know.

BridgeAR · 2017-09-19T22:20:55Z

Ping @Trott do you want to follow up on this? I would otherwise close this.

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification.

Trott · 2017-09-20T03:00:45Z

Ping @Trott do you want to follow up on this? I would otherwise close this.

I rebased, resolved conflicts, fixed all nits, and pushed back up to Joyee's branch. I think this is ready to land.

@cjihrig Can you review and clear your request for changes if this addresses your concerns? I'm not sure if the new wording I settled on is OK by you or not.

@joyeecheung Can you review and confirm that what I've done here meets your intentions?

@nodejs/documentation PTAL

BridgeAR · 2017-09-22T23:25:13Z

Landed in ba5a668

(I felt free to land this due to no response from @joyeecheung since a month)

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification. PR-URL: #12963 Refs: #12497 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification. PR-URL: nodejs/node#12963 Refs: nodejs/node#12497 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification. PR-URL: #12963 Refs: #12497 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 11, 2017

joyeecheung force-pushed the process-send-caveat branch from 8a3b2e1 to 6e47903 Compare May 11, 2017 05:43

joyeecheung added child_process Issues and PRs related to the child_process subsystem. process Issues and PRs related to the process subsystem. labels May 11, 2017

mscdex reviewed May 11, 2017

View reviewed changes

Trott reviewed May 11, 2017

View reviewed changes

cjihrig requested changes May 11, 2017

View reviewed changes

sam-github reviewed May 11, 2017

View reviewed changes

Trott force-pushed the process-send-caveat branch from 6e47903 to 9d99868 Compare September 20, 2017 02:57

cjihrig approved these changes Sep 20, 2017

View reviewed changes

jasnell approved these changes Sep 20, 2017

View reviewed changes

vsemozhetbyt approved these changes Sep 20, 2017

View reviewed changes

BridgeAR approved these changes Sep 22, 2017

View reviewed changes

BridgeAR closed this Sep 22, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: note caveats in process message serialization #12963

doc: note caveats in process message serialization #12963

joyeecheung commented May 11, 2017

mscdex May 11, 2017

joyeecheung May 11, 2017 •

edited

Loading

joyeecheung May 11, 2017

Trott May 11, 2017

mscdex May 11, 2017 •

edited

Loading

Trott May 11, 2017

mscdex May 11, 2017

Trott May 11, 2017

mscdex May 11, 2017

Trott May 11, 2017

Trott May 11, 2017

cjihrig left a comment

cjihrig May 11, 2017

sam-github May 11, 2017

BridgeAR commented Aug 26, 2017

Trott commented Aug 27, 2017

BridgeAR commented Sep 19, 2017

Trott commented Sep 20, 2017

BridgeAR commented Sep 22, 2017

doc: note caveats in process message serialization #12963

doc: note caveats in process message serialization #12963

Conversation

joyeecheung commented May 11, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

joyeecheung May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 26, 2017

Trott commented Aug 27, 2017

BridgeAR commented Sep 19, 2017

Trott commented Sep 20, 2017

BridgeAR commented Sep 22, 2017

joyeecheung May 11, 2017 •

edited

Loading

mscdex May 11, 2017 •

edited

Loading