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

Child process/streams memory leak #4049

Closed
jhenninger opened this Issue Nov 27, 2015 · 2 comments

Comments

Projects
None yet
4 participants
@jhenninger

The following code is leaking memory. I tested this using node-inspector and taking heap snapshots in chrome.

var cp = require('child_process');

setInterval(function () {
  var p = cp.spawn('yes');

  p.stdout.read();
  p.kill();
}, 50);

The leak stops if the p.stdout.read(); line is removed.

Another test, requires weak:

var cp = require('child_process');
var weak = require('weak');

var c = 0;

function cb() {
  console.log(--c);
}

setInterval(function () {
  var p = cp.spawn('yes');

  ++c;

  p.stdout.read();
  p.kill();

  weak(p, cb);

  gc();

}, 50);

Running this with --expose-gc will display the the number of process instances that haven't been garbaged collected. The number will always be counting up and never decrease.

If the p.stdout.read() line is removed, there will always be only one process object waiting to be collected.

@ChALkeR ChALkeR added the memory label Nov 28, 2015

@davidvgalbraith

This comment has been minimized.

Show comment
Hide comment
@davidvgalbraith

davidvgalbraith Nov 29, 2015

+1. The leak also seems to go away if you replace p.stdout.read() with p.stdout.read(0) or p.stdin.read() or p.stderr.read().

+1. The leak also seems to go away if you replace p.stdout.read() with p.stdout.read(0) or p.stdin.read() or p.stderr.read().

davidvgalbraith pushed a commit to davidvgalbraith/node that referenced this issue Nov 30, 2015

Dave
child_process.flushStdio: resume _consuming streams
When a client calls read() with a nonzero argument
on a Socket, that Socket sets this._consuming to true.
It never sets this._consuming back to false.
child_process.flushStdio currently doesn't flush
any streams where _consuming is truthy. But that means
that it never flushes any stream that has ever been read from.
This prevents a child process from ever closing if one of
its streams has been read from, causing issue #4049.

child_process.flushStdio should flush streams even if their
_consuming is set to true. Then it will close even after a read.
@davidvgalbraith

This comment has been minimized.

Show comment
Hide comment
@davidvgalbraith

davidvgalbraith Nov 30, 2015

I found this one interesting and untoward, so I took a crack at it: #4071.

I found this one interesting and untoward, so I took a crack at it: #4071.

@cjihrig cjihrig closed this in 34b535f Dec 1, 2015

rvagg added a commit that referenced this issue Dec 5, 2015

child_process: flush consuming streams
When a client calls read() with a nonzero argument
on a Socket, that Socket sets this._consuming to true.
It never sets this._consuming back to false.
ChildProcess.flushStdio() currently doesn't flush
any streams where _consuming is truthy. But, that means
that it never flushes any stream that has ever been read from.
This prevents a child process from ever closing if one of
its streams has been read from, causing issue #4049. This
commit allows consuming streams to be flushed, and the
child process to emit a close event.

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

MylesBorins added a commit that referenced this issue Jan 13, 2016

child_process: flush consuming streams
When a client calls read() with a nonzero argument
on a Socket, that Socket sets this._consuming to true.
It never sets this._consuming back to false.
ChildProcess.flushStdio() currently doesn't flush
any streams where _consuming is truthy. But, that means
that it never flushes any stream that has ever been read from.
This prevents a child process from ever closing if one of
its streams has been read from, causing issue #4049. This
commit allows consuming streams to be flushed, and the
child process to emit a close event.

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

MylesBorins added a commit that referenced this issue Jan 19, 2016

child_process: flush consuming streams
When a client calls read() with a nonzero argument
on a Socket, that Socket sets this._consuming to true.
It never sets this._consuming back to false.
ChildProcess.flushStdio() currently doesn't flush
any streams where _consuming is truthy. But, that means
that it never flushes any stream that has ever been read from.
This prevents a child process from ever closing if one of
its streams has been read from, causing issue #4049. This
commit allows consuming streams to be flushed, and the
child process to emit a close event.

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

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

Dave Michael Scovetta
child_process: flush consuming streams
When a client calls read() with a nonzero argument
on a Socket, that Socket sets this._consuming to true.
It never sets this._consuming back to false.
ChildProcess.flushStdio() currently doesn't flush
any streams where _consuming is truthy. But, that means
that it never flushes any stream that has ever been read from.
This prevents a child process from ever closing if one of
its streams has been read from, causing issue #4049. This
commit allows consuming streams to be flushed, and the
child process to emit a close event.

Fixes: nodejs#4049
PR-URL: nodejs#4071
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