This repository has been archived by the owner. 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

Projects
None yet
5 participants
Member

indexzero commented May 9, 2012

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 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.

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 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.

Member

bnoordhuis commented May 14, 2012

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

2012.05.15 Version 0.6.18 (stable)
* 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

2012.05.15 Version 0.6.18 (stable)
* 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.