Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Test fork std{out,err} #2442

Closed
wants to merge 1 commit into from
Closed

Conversation

mmalecki
Copy link

This test currently fails.

@piscisaureus said that child_process.fork should return a child with stdout and stderr, so I assume it's a bug. Currently child has only stdin property.

@@ -0,0 +1,4 @@
var N = 0;

Choose a reason for hiding this comment

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

What's this for?

Copy link
Author

Choose a reason for hiding this comment

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

Fuck, sec (edit: wanted it to write multiple multiple messages, but I ditched it, it's irrelevant here).

@ry
Copy link

ry commented Dec 31, 2011

forked child processes share the stdout and stderr with the parent, use the ipc channel

@mmalecki
Copy link
Author

@ry I don't want to use it for communication but for logging.

@AndreasMadsen
Copy link
Member

@ry The documentation clearly say:

Child processes always have three streams associated with them. child.stdin, child.stdout, and child.stderr.

If this is not a documentation issue, then stdout and stderr should be piped to the child.stderr and child.stdout and then pumped to the parent stderr and stdout.

In node v0.7 the silent option would then be used to determin if the pump should be created.

@mmalecki
Copy link
Author

mmalecki commented Jan 2, 2012

I have a fix upcoming, will pull request soon and link here

mmalecki added a commit to mmalecki/node that referenced this pull request Jan 3, 2012
Previously using these streams was not possible because of making them
use given file descriptors. This solution is more generic - it uses
`pipe` function to pipe fork's output into parent's `stdout` and
`stderr` unless `silent: true` is set as an option.

This doesn't break any existing API and it makes `fork` more compatible
with `spawn`.

Please note that fork's `stdout` and `stderr` aren't meant to be used as
communication channels - use `fork(...).send()` instead.

Fixes nodejs#2442.
@mmalecki
Copy link
Author

mmalecki commented Jan 3, 2012

This test passes with #2454 applied.

@bnoordhuis
Copy link
Member

@mmalecki Can I close this? If I read the bug report right, this is no longer an issue with v0.8.

@mmalecki
Copy link
Author

mmalecki commented Aug 8, 2012

@bnoordhuis you can close it or pull it (if there's no test for that).

@isaacs
Copy link

isaacs commented Aug 8, 2012

I'm ok with the test, but are you sure there isn't already a test we can just add stdout/stderr checking into?

@bnoordhuis
Copy link
Member

The test needs a patch to work with v0.8 (see below) but we already have a test for that, simple/test-child-process-silent.

diff --git a/test/simple/test-child-process-fork-stdout.js b/test/simple/test-child-process-fork-stdout.js
index 8f92ccb..05967e5 100644
--- a/test/simple/test-child-process-fork-stdout.js
+++ b/test/simple/test-child-process-fork-stdout.js
@@ -27,7 +27,8 @@ var common = require('../common');
 var stdout = '';
 var stderr = '';

-var child = fork(path.join(common.fixturesDir, 'stdout-stderr-output'));
+var filename = path.join(common.fixturesDir, 'stdout-stderr-output');
+var child = fork(filename, {silent:true});

 child.stdout.on('data', function (chunk) {
   stdout += chunk;

@bnoordhuis bnoordhuis closed this Aug 8, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants