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

Cannot work with stdin in a sub-subprocess with require('child_process').fork() #3240

Closed
indexzero opened this issue May 9, 2012 · 4 comments

Comments

@indexzero
Copy link

I'm pretty sure this is a bug. @substack and I spent some time tracking this down when his application wouldn't deploy to nodejitsu:

Reproduce it

  1. Clone this application: http://github.com/substack/pix
  2. Add this script to it: https://gist.github.com/2640933

In (2) I added a layer of indirection which spawns server.js, once with .fork() and once with .spawn().

Here's a quick rundown of the process tree(s):

  1. Just running node server.js
  server.js
   `-- worker.js
   `-- worker.js
  1. Running node spawn.js
  spawn.js
    `-- server.js
          `-- worker.js
          `-- worker.js

So just that one level of indirection with .fork() breaks all piping of process.stdin.

@piscisaureus @isaacs @bnoordhuis You guys have any thoughts here? I'm stumped why this is happening, but I know it is.

@isaacs
Copy link

isaacs commented May 9, 2012

There are a few bugs inpix.

child_process.fork doesn't preserve the env by default, and server.js is spawning just 'node' rather than process.execPath, and not specifying a PATH, so it's failing with execvp(): No such file or directory.

However, this error isn't seen, because the stderr from the worker in server.js is not ever examined, and spawn doesn't inherit the output handles by default.

Pix's server.js doesn't examine the exit code in the 'exit' event listener, so this also obscures errors.

AFAICT, everything in node is operating as designed. However, it would be nice if the env was handled the same way by fork as it is by spawn.

@ghost
Copy link

ghost commented May 12, 2012

I made a gist to demonstrate the issue more cleanly and to show that this is not a problem with the environment variables:
https://gist.github.com/2664487

The worker.js is getting run because working... gets printed but no 'data' events come through from spawn.js to worker.js like they should.

@rf
Copy link

rf commented May 13, 2012

After some analysis, I've concluded that I agree with @isaacs; everything in node is operating as designed. Because the environment isn't cleared before spawn.js spawns a worker.js, worker.js gets an environment with a NODE_CHANNEL_FD value. This value is passed to help setup the parent -> child communication circuitry. Since spawn.js was created with a call to child_process.fork(), it has this variable set.

This value then gets to the third process: worker.js. It sees the NODE_CHANNEL_FD environment variable and attempts to setup the communication circuitry, which, of course, it cannot, because it was in fact not created with a call to child_process.fork().

The code works as intended if we simply clear the environment in spawn.js:

var ps = spawn(process.execPath, ['worker.js' ], {env: {}});

It may, however, be reasonable to clear NODE_CHANNEL_FD when spawn()ing so that processes spawn()ed from a process which came from a fork() won't try to setup the communication channel.

@bnoordhuis
Copy link
Member

What @russfrank said, the inherited NODE_CHANNEL_FD is the culprit. It's fixed in bd90717.

isaacs added a commit that referenced this issue May 14, 2012
* windows: skip GetFileAttributes call when opening a file (Bert Belder)

* crypto: add PKCS12/PFX support (Sambasiva Suda)

* #3240: child_process: delete NODE_CHANNEL_FD from env in spawn (Ben Noordhuis)

* windows: add test for path.normalize with UNC paths (Bert Belder)

* windows: make path.normalize convert all slashes to backslashes (Bert Belder)

* fs: Automatically close FSWatcher on error (Bert Belder)

* #3258: fs.ReadStream.pause() emits duplicate data event (koichik)

* pipe_wrap: don't assert() on pipe accept errors (Ben Noordhuis)

* Better exception output for module load and process.nextTick (Felix Geisendörfer)

* zlib: fix error reporting (Ben Noordhuis)

* http: Don't destroy on timeout (isaacs)

* #3231: http: Don't try to emit error on a null'ed req object (isaacs)

* #3236: http: Refactor ClientRequest.onSocket (isaacs)
isaacs added a commit that referenced this issue May 15, 2012
* windows: skip GetFileAttributes call when opening a file (Bert Belder)

* crypto: add PKCS12/PFX support (Sambasiva Suda)

* #3240: child_process: delete NODE_CHANNEL_FD from env in spawn (Ben Noordhuis)

* windows: add test for path.normalize with UNC paths (Bert Belder)

* windows: make path.normalize convert all slashes to backslashes (Bert Belder)

* fs: Automatically close FSWatcher on error (Bert Belder)

* #3258: fs.ReadStream.pause() emits duplicate data event (koichik)

* pipe_wrap: don't assert() on pipe accept errors (Ben Noordhuis)

* Better exception output for module load and process.nextTick (Felix Geisendörfer)

* zlib: fix error reporting (Ben Noordhuis)

* http: Don't destroy on timeout (isaacs)

* #3231: http: Don't try to emit error on a null'ed req object (isaacs)

* #3236: http: Refactor ClientRequest.onSocket (isaacs)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants