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

doc: document that `'ipc'` is required with fork stdio option #8290

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jasnell
Member

jasnell commented Aug 27, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

document that 'ipc' is required with fork stdio option

Fixes: #8167

Show outdated Hide outdated doc/api/child_process.md
@@ -290,6 +290,17 @@ parent process using the file descriptor (fd) identified using the
environment variable `NODE_CHANNEL_FD` on the child process. The input and
output on this fd is expected to be line delimited JSON objects.
When specifying the [`stdio`][] option, the value *must* be a JSON Array

This comment has been minimized.

@cjihrig

cjihrig Aug 27, 2016

Contributor

JSON Array -> array

@cjihrig

cjihrig Aug 27, 2016

Contributor

JSON Array -> array

Show outdated Hide outdated doc/api/child_process.md
@@ -290,6 +290,17 @@ parent process using the file descriptor (fd) identified using the
environment variable `NODE_CHANNEL_FD` on the child process. The input and
output on this fd is expected to be line delimited JSON objects.
When specifying the [`stdio`][] option, the value *must* be a JSON Array
containing at least one item with value `'ipc'` or a `TypeError` will be

This comment has been minimized.

@cjihrig

cjihrig Aug 27, 2016

Contributor

Shouldn't it be exactly one 'ipc'? I added test in ae4ce9f that fails if you try to setup multiple IPC channels.

@cjihrig

cjihrig Aug 27, 2016

Contributor

Shouldn't it be exactly one 'ipc'? I added test in ae4ce9f that fails if you try to setup multiple IPC channels.

This comment has been minimized.

@jasnell

jasnell Aug 27, 2016

Member

Awesome, no 'ipc' == TypeError, too many 'ipc''s == Error. Yay consistency!

@jasnell

jasnell Aug 27, 2016

Member

Awesome, no 'ipc' == TypeError, too many 'ipc''s == Error. Yay consistency!

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

I would just add this into the existing fork() stdio documentation, instead of creating a new paragraph, but I don't feel really strongly about it.

Contributor

cjihrig commented Aug 27, 2016

I would just add this into the existing fork() stdio documentation, instead of creating a new paragraph, but I don't feel really strongly about it.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

Fixed with exactly. I'd rather keep this with the fork method given that it's where this error is most likely to be seen.

Member

jasnell commented Aug 27, 2016

Fixed with exactly. I'd rather keep this with the fork method given that it's where this error is most likely to be seen.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

Yea, the fork() method has its own documentation for stdio :-)

Contributor

cjihrig commented Aug 27, 2016

Yea, the fork() method has its own documentation for stdio :-)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

oh.. you mean up in the options list?

Member

jasnell commented Aug 27, 2016

oh.. you mean up in the options list?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

Yea, but I won't fight the existing approach.

Contributor

cjihrig commented Aug 27, 2016

Yea, but I won't fight the existing approach.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

heh... I can do that. I thought you meant moving it down into the more extensive documentation under spawn

Member

jasnell commented Aug 27, 2016

heh... I can do that. I thought you meant moving it down into the more extensive documentation under spawn

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

LGTM

Contributor

cjihrig commented Aug 27, 2016

LGTM

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 28, 2016

Member

LGTM

Member

benjamingr commented Aug 28, 2016

LGTM

jasnell added a commit that referenced this pull request Aug 29, 2016

doc: `'ipc'` is required with fork stdio option
Fixes: #8167
PR-URL: #8290
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2016

Member

Landed in f10e1ed

Member

jasnell commented Aug 29, 2016

Landed in f10e1ed

@jasnell jasnell closed this Aug 29, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

doc: `'ipc'` is required with fork stdio option
Fixes: nodejs#8167
PR-URL: nodejs#8290
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

doc: `'ipc'` is required with fork stdio option
Fixes: #8167
PR-URL: #8290
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment