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

doc: improve child_process.markdown copy #4383

Closed

Conversation

Projects
None yet
5 participants
@jasnell
Copy link
Member

commented Dec 21, 2015

General improvements to child_process.markdown

@cjihrig

View changes

doc/api/child_process.markdown Outdated
assert.equal(child.stdio[2], null);
assert.equal(child.stdio[2], child.stderr);
Pipes for `stdin`, `stdout` and `stderr` are established between the parent
Node.js process and the created child. It is possible to stream data through

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Maybe replace "created" with "spawned" here.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

They are only established when spawn is called with pipe as an option, or fork with silent: true. Unless I misunderstand the diff, this statement seems much more sweeping than is accurate.

This comment has been minimized.

Copy link
@jasnell

jasnell Dec 23, 2015

Author Member

I will clarify that this is the default behavior

@cjihrig

View changes

doc/api/child_process.markdown Outdated
* The child process is terminated using the signal identified by `killSignal`
(the default is `'SIGTERM'`).
* The `maxBuffer` specifies the largest amount of data (in bytes) allowed on
stdout or stderr - if this value is exceeded then the child process is

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Instead of - if this value..., just start a new sentence.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
`killSignal` (default: `'SIGTERM'`). `maxBuffer` specifies the largest
amount of data (in bytes) allowed on stdout or stderr - if this value is
exceeded then the child process is killed.
* If `timeout` is greater than `0`, then the child process will be terminated

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

It might be better to keep the original kill terminology so it's clearer how the process is terminated.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
subshell but rather the specified file directly. This makes it slightly
leaner than [`child_process.exec()`][]. It has the same options.
The `child_process.execFile()` method is similar to [`child_process.exec()`][]
except that it does not first spawn a shell. Rather the specified `command` is

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Comma after "rather"

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

and since it doesn't spawn a shell, cannot run node/npm installed .cmd wrappers for node scripts, such as npm!

@cjihrig

View changes

doc/api/child_process.markdown Outdated
leaner than [`child_process.exec()`][]. It has the same options.
The `child_process.execFile()` method is similar to [`child_process.exec()`][]
except that it does not first spawn a shell. Rather the specified `command` is
spawned directly as a new process makes it slightly more efficient than

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

new process, making

@cjihrig

View changes

doc/api/child_process.markdown Outdated
These child Node.js processes are still whole new instances of V8. Assume at
least 30ms startup and 10mb memory for each new Node.js. That is, you cannot
create many thousands of them.
Spawned child Node.js processes include their own whole new instances of V8.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Spawned Node.js child processes include separate instances of V8.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

v8 seems a bit of a red herring: also new instances of libc, openssl, etc., etc. Can we say something about being a completely independent Node.js process? Don't we already say that, actually?

@cjihrig

View changes

doc/api/child_process.markdown Outdated
Launches a new process with the given `command`, with command line arguments in
`args`. If omitted, `args` defaults to an empty Array.
The `child_process.spawn()` method spawns a new process using the given
`command`, with command line arguments in `args`. If omitted, `args` defaults

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

There are two spaces between "with" and "command"

@cjihrig

View changes

doc/api/child_process.markdown Outdated
`args`. If omitted, `args` defaults to an empty Array.
The `child_process.spawn()` method spawns a new process using the given
`command`, with command line arguments in `args`. If omitted, `args` defaults
to an empty Array.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

I think we can lowercase Array.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
disabled).
On Windows, setting `options.detached` to `true` makes it possible for the
child process to continue running after the parent exits. The child will have
its own console window. *Once for a child process, this cannot be disabled*.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

The last bit doesn't make sense. Maybe revert to the old (this cannot be disabled).

This comment has been minimized.

Copy link
@jasnell

jasnell Dec 23, 2015

Author Member

It's supposed to have been, Once enabled for a child process, it cannot be disabled

@cjihrig

View changes

doc/api/child_process.markdown Outdated
the leader of a new process group and session. Note that child processes may
continue running after the parent exits whether they are detached or not. See
`setsid(2)` for more information.
On non-Windows platforms, if `options.detached` is set to *true*, the child

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Backticks around true instead of asterisks for consistency.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
@@ -605,14 +342,23 @@ controlling terminal.

#### options.stdio

As a shorthand, the `stdio` argument may be one of the following strings:
The `options.stdio` option is used to configure the Pipes that are established

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Pipes can be lowercased.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
* `'pipe'` - `['pipe', 'pipe', 'pipe']`, this is the default value
* `'ignore'` - `['ignore', 'ignore', 'ignore']`
* `'inherit'` - `[process.stdin, process.stdout, process.stderr]` or `[0,1,2]`
* `'pipe'` - equivalent to `['pipe', 'pipe', 'pipe']`

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Probably worth leaving the note about this being the default value.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
Otherwise, the `'stdio'` option to [`child_process.spawn()`][] is an array where each
index corresponds to a fd in the child. The value is one of the following:
Otherwise, the value of `option.stdio` is an array where each index corresponds
to an fd in the child. The fds 0, 1 and 2 correspond to stdin, stdout,

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Comma after "1"

@cjihrig

View changes

doc/api/child_process.markdown Outdated
Otherwise, the value of `option.stdio` is an array where each index corresponds
to an fd in the child. The fds 0, 1 and 2 correspond to stdin, stdout,
and stderr, respectively. Additional fds can be specified to create additional
Pipes between the parent and child. The value is one of the following:

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Lowercase pipes

@cjihrig

View changes

doc/api/child_process.markdown Outdated
will always open fd 0 - 2 for the processes it spawns. When any of these is
ignored Node.js will open `/dev/null` and attach it to the child's fd.
If the child writes JSON messages to this file descriptor, the
`ChildProcess.on('message')` event will be triggered in the parent. If the

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

I think it would be an event handler that would be triggered.

@cjihrig

View changes

doc/api/child_process.markdown Outdated
ignored Node.js will open `/dev/null` and attach it to the child's fd.
If the child writes JSON messages to this file descriptor, the
`ChildProcess.on('message')` event will be triggered in the parent. If the
child is Node.js process, the presence of an IPC channel will enable

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Add "a" between "is" and "Node.js"

@cjihrig

View changes

doc/api/child_process.markdown Outdated
`child_process.exec()` with the exception that the method will not return until
the child process has fully closed. When a timeout has been encountered and
`killSignal` is sent, the method won't return until the process has completely
exited. *Note that the child process intercepts and handles the `SIGTERM`

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Add "if" between "that" and "the"

the `SIGTERM` signal and doesn't exit, your process will wait until the child
The `child_process.spawnSync()` method is generally identical to
`child_process.spawn()` with the exception that the function will not return
until the child process has fully closed. When a timeout has been encountered

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

These repetitive notes could probably be moved to the general Synchronous Process Creation section.

This comment has been minimized.

Copy link
@jasnell

jasnell Dec 23, 2015

Author Member

Considered that but figured it would be better to keep things together a bit more. How about we look at this again once the larger set of changes have been landed?

@cjihrig

View changes

doc/api/child_process.markdown Outdated

Instances of the `ChildProcess` class represent spawned child processes.

`ChildProcess` is an [`EventEmitter`][].

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

This could be combined with the previous sentence. "Instance of the ChildProcess class are EventEmitters that represent spawned child processes."

@cjihrig

View changes

doc/api/child_process.markdown Outdated
which can be piped to and from.

Instances of `ChildProcess` are not intended to be created directly. Rather,
Use the [`child_process.spawn()`][], [`child_process.exec()`][],

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Lowercase "Use"

@cjihrig

View changes

doc/api/child_process.markdown Outdated
Instances of `ChildProcess` are not intended to be created directly. Rather,
Use the [`child_process.spawn()`][], [`child_process.exec()`][],
[`child_process.execFile()`][], or [`child_process.fork()`][] methods (or
their synchronous counterparts) to create instances of `ChildProcess`.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

I don't think the synchronous versions actually return an instance of ChildProcess. The following logs false.

const child = cp.spawnSync('ls');
console.log(child instanceof cp.ChildProcess);

This comment has been minimized.

Copy link
@jasnell

jasnell Dec 23, 2015

Author Member

hmm.. good point. It looks like the return value of the sync variants isn't documented at all currently :-/

@cjihrig

View changes

doc/api/child_process.markdown Outdated
was killed by the parent.

The `'close'` event is emitted when the stdio streams of a child process have
all terminated. This is distinct from the `'exit'` event, since multiple

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Replace terminated with "been closed"

@cjihrig

View changes

doc/api/child_process.markdown Outdated
const server = require('net').createServer();
server.on('connection', (socket) => {

// if this is special priority

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 22, 2015

Contributor

Can you capitalize the first letter of the comments.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2015

Left a bunch of comments, but generally LGTM

@sam-github

View changes

doc/api/child_process.markdown Outdated
and asynchronous alternatives to `child_process.spawn()`, each of which are
documented fully [below][]:

* `child_process.exec()`: spawns a shell and runs a command within that shell.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

... "returning the stdout and stderr when the command completes". <--- that last is the very important distinction between the exec* and the spawn/fork.

@sam-github

View changes

doc/api/child_process.markdown Outdated
* `child_process.execFile()`: similar to `child_process.exec()` except that
it spawns the command directly without first spawning a shell.
* `child_process.fork()`: spawns a new Node.js process and invokes a
specified module.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

"with a control channel set up for sending messages between parent and child"

@sam-github

View changes

doc/api/child_process.markdown Outdated
If a `callback` function is provided, it is called with the arguments
`(error, stdout, stderr)`. On success, `error` will be `null`. On error,
`error` will be an instance of [`Error`][]. The `error.code` property will be
the exit code of the child process while `error.signal` will be set to the

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

it's not obvious that a non-zero exit code is considered an "error"

@@ -376,7 +84,8 @@ callback or returning an EventEmitter).
* `stderr` {Buffer}
* Return: ChildProcess object

Runs a command in a shell and buffers the output.
Spawns a shell then executes the `command` within that shell, buffering any
generated output.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

The distinction between exec and execFile is just efficiency, pretty much, on Unix. On Windows, the distinction is MUCH more imporant: .bat and .cmd files are NOT executable. So for example, all npm installed node CLIs can NOT be execFile() or spawn() ed on Windows... they can only be run by a shell, since cmd.exe knows how to run them.

Its a huge portability pitfall, and the current docs gloss over that.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

addressed above

@sam-github

View changes

doc/api/child_process.markdown Outdated
be launched using `child_process.execFile()` (or even `child_process.spawn()`).
When running on Windows, `.bat` and `.cmd` files can only be invoked using
either `child_process.exec()` or by spawning `cmd.exe` and passing the `.bat`
or `.cmd` file as an argument.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

(which is what child_process.exec() does). <-- worth mentioning? to demystify?

@sam-github

View changes

doc/api/child_process.markdown Outdated
If the child writes JSON messages to this file descriptor, the
`ChildProcess.on('message')` event handler will be triggered in the parent.
If the child is a Node.js process, the presence of an IPC channel will enable
`process.send()`, `process.disconnect()`, and `process.on('message')` within

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

sorry, also process.on('disconnect') is available in the child.

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 23, 2015

Member

also, re: unref(), children start off with the control channel unrefed until process.on('disconnect') is called, then it gets refed. Also, I think when process.send() is called. Sorry, I'm going from memory here. Not sure where that info belongs, maybe in the docs for process.on('disconnect')?

@sam-github

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

@jasnell Looks like you addressed all my comments, I remembered a couple other useful tidbits of info, though.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2015

@sam-github ... ok, pushed another set of edits.

@Trott Trott force-pushed the nodejs:master branch to 082cc8d Dec 27, 2015

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2015

@nodejs/collaborators ... can I ask for some additional review and sign off on this? Thank you!

doc: improve child_process.markdown copy
General improvements to child_process.markdown

@jasnell jasnell force-pushed the jasnell:doc-child-process-improvements branch to f53b65d Dec 28, 2015

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2015

LGTM

sending messages between parent and child.
* `child_process.execSync()`: a synchronous version of
`child_process.exec()` that *will* block the Node.js event loop.
* `child_process.execFileSync()`: a synchronous version of

This comment has been minimized.

Copy link
@ryansobol

ryansobol Dec 30, 2015

Contributor

Does it make sense to list the Sync versions right below their asynchronous counterpart?

This comment has been minimized.

Copy link
@jasnell

jasnell Dec 30, 2015

Author Member

I'd prefer to keep them grouped by async vs sync

jasnell added a commit that referenced this pull request Dec 30, 2015

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: #4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2015

Landed in 7d5c1b5. Can make additional improvements if necessary in a separate PR

@jasnell jasnell closed this Dec 30, 2015


const exec = require('child_process').execFile;

This comment has been minimized.

Copy link
@ryansobol

ryansobol Dec 30, 2015

Contributor

Sadly, this code example doesn't work even on Unix-like platforms. Since there's no shell wrapper,

  1. file must be an absolute path to executable.
  2. args must contain all the command line arguments for file.
  3. All paths within args must be absolute as well.
  4. Stdio redirection is not possible.

Here's an alternative I whipped up that should work on all Unix-like platforms. As always, feel free to use none, some, or all of it.

const execFile = require('child_process').execFile;
const child = execFile('/bin/cat', ['/etc/paths'], (error, stdout, stderr) => {
  if (error) {
    throw error;
  }
  console.log(stdout);
});
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2015

@ryansobol ... can I ask you for a quick PR to update the example? :-) Thank you!

@ryansobol

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2015

@jasnell No problem. See #4504

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 6, 2016

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: nodejs#4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jan 7, 2016

@jasnell this is not merging cleanly, can you backport this please.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2016

@thealphanerd ..yes, I'll work on backporting all of these kind of doc changes. They may not all make it into v4.2.5 but I'll work on them

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2016

@thealphanerd ... I'll be working on porting this to LTS early next week.

jasnell added a commit to jasnell/node that referenced this pull request Feb 19, 2016

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: nodejs#4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Feb 22, 2016

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: #4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Feb 22, 2016

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: #4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 2, 2016

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: #4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

doc: improve child_process.markdown copy
General improvements to child_process.markdown

PR-URL: nodejs#4383
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.