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

process: make serialization function configurable #6400

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 26, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

process

Description of change

By default process.send() serializes messages using vanilla JSON.stringify(). This commit makes the serialization function configurable.

Refs: #6300

By default process.send() serializes messages using vanilla
JSON.stringify(). This commit makes the serialization function
configurable.

Refs: nodejs#6300
@bnoordhuis
Copy link
Member

LGTM-ish. The code changes themselves LGTM but:

  1. The documentation should clarify that the expected output is still JSON.
  2. This seems like half a fix as long as you can't configure the deserializer.
  3. If the deserializer is configurable, we need to come up with a better way to separate messages than newlines. Length-prefixed messages?

'use strict';
const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
Copy link
Member

Choose a reason for hiding this comment

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

Not that it matters to much, but assert and cp could be required only in the parent.

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 26, 2016

I thought about the deserializer configuration, but no good ideas came to mind. Theoretically, you could specify a different serializer for every call to send(). However unlikely that is, how would you handle that with the deserializer?

@claudiorodriguez claudiorodriguez added child_process Issues and PRs related to the child_process subsystem. process Issues and PRs related to the process subsystem. labels Apr 26, 2016
@sam-github
Copy link
Contributor

Nothing in this PR, code, comments, or the docs suggests a reason to do this. The input is fixed, the output is fixed (must be JSON)... but you want to replace the built-in serializer because.... why?

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 26, 2016

My reason is that forcing JSON.stringify() to serialize data is limiting. As it is currently implemented, you don't even have access to stringify()'s replacer function. This means you have to serialize/process it yourself first, and then send it through stringify() again anyway.

Additionally, as brought up in #6300 and other places, the JSON.stringify() and JSON.parse() are a bit of bottlenecks. As discussed above, it would be ideal to make the deserializer configurable as well, then JSON wouldn't be required at all.

@eljefedelrodeodeljefe
Copy link
Contributor

LGTM. Good first step. Also utilizing .stringify() features is very effective for chopping of data, see here.

@vkurchatkin
Copy link
Contributor

What's the point if deserializer is still the same? One can just define toJSON

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 26, 2016

I agree, the deserializer should be configurable. Giving it some thought.


If Node.js was not spawned with an IPC channel, `process.send()` will be undefined.
If Node.js was not spawned with an IPC channel, `process.send()` will be
undefined.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please include some detail about the serializer function. What arguments will be passed? What should it return? What happens if it throws? If it shouldn't throw, how should it handle errors? etc.

@jasnell
Copy link
Member

jasnell commented Apr 27, 2016

This stuff starts to get very complicated with configurable serializers and deserializers but it can also be very handy. If you're going to go this route, I would recommend using a simple binary framing for the messages. A small header frame would be used to identify the serializer that was used so that an appropriate deserializer can be selected on the receiving end. Then use a length prefix for the actual payload.

If the receiving end gets a message it cannot deserialize, we would need to decide if that's an error or not. Also, would we want to require the messages to still be JSON or at least text based? One could easily imagine someone wanting to send a fully binary message, in which case the serializer could return a Buffer object (it's a fairly slippery slope).

I'm certainly not against allowing for multiple serializers we just need to be deliberate about how we do so (and about just how much freedom we give to those).

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 27, 2016
@cjihrig cjihrig closed this Apr 27, 2016
@eljefedelrodeodeljefe
Copy link
Contributor

Are you continuing somewhere else?

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 27, 2016

Not right now.

@rumkin
Copy link
Contributor

rumkin commented May 31, 2016

@cjihrig I think deserialiser should be a node argv option. Something like ipc-serialiser:

node --ipc-serialiser=module-name app.js ...

It should to specify node module which realises serialisation interface with methods serialise and deserialise.

Or better to add preload argv option which allow to specify one or more initially loaded modules and pass serialiser module to it.

@addaleax
Copy link
Member

Or better to add preload argv option which allow to specify one or more initially loaded modules and pass serialiser module to it.

There already is the -r CLI option for pre-loading modules, e.g. node -r ./preloaded.js main.js… is that what you’re looking for?

@rumkin
Copy link
Contributor

rumkin commented May 31, 2016

@addaleax Shame on me 😕 ... Yep that's what I was talking about. Thanks.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 31, 2016

@rumkin the -r flag only partially does what you asked for. It preloads the module, but there is not API for changing the IPC serialization functions.

@rumkin
Copy link
Contributor

rumkin commented Jun 1, 2016

@cjihrig But I can require such module which will register serialiser. Registration could be made with method process.registerSerialiser. And it shouldn't be called twice without calling process.unregisterSerialiser.

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. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants