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.

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

Child Process send incorrectly serializes values #5453

Closed
jacobp100 opened this issue Feb 26, 2016 · 9 comments
Closed

Child Process send incorrectly serializes values #5453

jacobp100 opened this issue Feb 26, 2016 · 9 comments
Labels
child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors.

Comments

@jacobp100
Copy link

Code

index.js

const childProcess = require('child_process').fork(__dirname + '/worker.js');
childProcess.send('get-infinity');
childProcess.on('message', (valuePair) => {
  console.log(valuePair);
});

worker.js

process.on('message', () => {
  process.send(['Infinity', Infinity]);
  process.send(['NaN', NaN]);
});

Expected Result

['Infinity', Infinity]
['NaN', NaN]

Actual Result

['Infinity', null]
['NaN', null]
  • Version: _output of node -v

v5.6.0

  • Platform: _either uname -a output, or if Windows, version and 32-bit or
    64-bit

Linux tido-tchaikovsky 3.19.0-51-generic #57~14.04.1-Ubuntu SMP Fri Feb 19 14:36:55 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

  • Subsystem: optional. if known - please specify affected core module name

child_process

@cjihrig
Copy link
Contributor

cjihrig commented Feb 26, 2016

The message is stringified prior to sending using JSON.stringify():

> JSON.stringify(NaN);
'null'
> JSON.stringify(Infinity);
'null'

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Feb 26, 2016
@evanlucas
Copy link
Contributor

This is actually according to the specification. Please see http://www.ecma-international.org/ecma-262/6.0/#sec-json.stringify, specifically Note 4.

NOTE 4 Finite numbers are stringified as if by calling ToString(number). NaN and Infinity regardless of sign are represented as the String null.

Closing as this is working as designed. Thanks!

@jacobp100
Copy link
Author

I know it's according to the json spec. But i don't think this issue should be closed. Firstly, it does not specify in the documentation for process that it uses json stringify for data transfer. Secondly, there are other ways to serialise data that wouldn't suffer from this.

@evanlucas evanlucas reopened this Feb 28, 2016
@evanlucas
Copy link
Contributor

@jacobp100 Fair enough. At this point, changing the way serialization between processes is done would be a pretty large undertaking. I do recall that it has been considered though. For now, I think that a doc update would suffice. Would you be willing to submit a Pull Request with that update? Thanks!

@bnoordhuis
Copy link
Member

The documentation does mention in a few places that JSON is used but I agree it could be more explicit, particularly the reference documentation for process.send() and ChildProcess#send().

Secondly, there are other ways to serialise data that wouldn't suffer from this.

Sure, but we're not using that, have never used that in the past and I don't think this particular issue is important enough to merit writing a high-performance serializer/deserializer from scratch. That's without even getting into the issue of backwards compatibility.

@abenhamdine
Copy link

@jacobp100 do you plan to submit a PR for a doc update ? If no, I'm volunteer to do it as I'm interested in this.

@evanlucas evanlucas added the good first issue Issues that are suitable for first-time contributors. label Feb 29, 2016
@jacobp100
Copy link
Author

Go for it!

mithun-daa added a commit to mithun-daa/node that referenced this issue Mar 15, 2016
Explicitly call out that the send method on both process and child uses JSON.stringify to serialize the message
Fixes nodejs#5453
evanlucas pushed a commit that referenced this issue Mar 15, 2016
process.send and child.send use JSON.stringify to serialize
the message.

Fixes: #5453
PR-URL: #5723
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Mar 16, 2016
process.send and child.send use JSON.stringify to serialize
the message.

Fixes: #5453
PR-URL: #5723
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 17, 2016
process.send and child.send use JSON.stringify to serialize
the message.

Fixes: #5453
PR-URL: #5723
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
process.send and child.send use JSON.stringify to serialize
the message.

Fixes: #5453
PR-URL: #5723
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
process.send and child.send use JSON.stringify to serialize
the message.

Fixes: #5453
PR-URL: #5723
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@SunflowerPKU
Copy link

Hi @abenhamdine ,

I am a Ph.D. student. I am doing research about helping newcomers participate in OSS projects. I noticed that many projects are using labels such as 'good first issue/bug, difficulty/newcomer' for issues to recommend that newcomers start from these tasks.

I noticed that you tried to solve this issue, but unfortunately, you had not contributed successfully. I also found there are many newcomers feel difficult when submitting their first pr. Therefore, I want to optimize this mechanism, which needs your help. I have some questions and wish your valuable feedback.

  1. What problems did you meet when solving this issue?
  2. Do you think it is appropriate to recommend newcomers to solve this issue? and why?
  3. How do you think of the existing mechanism that adding certain labels to issues for helping newcomers participate in open source projects?

I am looking forward to hearing from you soon. Thank you very much!

@jacobp100 do you plan to submit a PR for a doc update ? If no, I'm volunteer to do it as I'm interested in this.

@abenhamdine
Copy link

abenhamdine commented May 30, 2019

Hi @abenhamdine ,

I am a Ph.D. student. I am doing research about helping newcomers participate in OSS projects. I noticed that many projects are using labels such as 'good first issue/bug, difficulty/newcomer' for issues to recommend that newcomers start from these tasks.

very interesting research !

I noticed that you tried to solve this issue, but unfortunately, you had not contributed successfully. I also found there are many newcomers feel difficult when submitting their first pr. Therefore, I want to optimize this mechanism, which needs your help. I have some questions and wish your valuable feedback.

  1. What problems did you meet when solving this issue?

No pb, the OP wanted to send a PR so finally I didn't handle the PR.

  1. Do you think it is appropriate to recommend newcomers to solve this issue? and why?

It was a doc issues and it's easier for newcomers to handle doc updates than actually modify the code.

  1. How do you think of the existing mechanism that adding certain labels to issues for helping newcomers participate in open source projects?

Yes, its a common practice to attach "good first issue" to issues, in many popular repositories
And it's a clear signal that the issue is not too complex and can be handled by a newcomer.
It's a good mecanism.

I am looking forward to hearing from you soon. Thank you very much!

@jacobp100 do you plan to submit a PR for a doc update ? If no, I'm volunteer to do it as I'm interested in this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants