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

EventEmitter memory leak warning #7

Closed
pushred opened this issue Jul 5, 2014 · 3 comments
Closed

EventEmitter memory leak warning #7

pushred opened this issue Jul 5, 2014 · 3 comments
Assignees

Comments

@pushred
Copy link

pushred commented Jul 5, 2014

I'm using gulp-run in conjunction with gulp-tap to pass the paths of files I'm watching on to a theme upload command in shopify_theme Ruby gem. Generally this works but I've started seeing the following warning as the number of files in my project increases. The uploads are still successful, but this is making for some noisy log output.

var gulp = require('gulp');
var sass = require('gulp-ruby-sass');
var run = require('gulp-run');
var tap = require('gulp-tap');

var paths = {
  styles: 'assets/styles/**/*'
};

var upload = function( file ){
  var splitPath = file.path.split('theme/').pop();
  run('theme upload ' + splitPath, { cwd: 'theme' }).exec();
};

gulp.task('sass', function(){
  gulp.src(paths.styles)
    .pipe(sass())
    .pipe(gulp.dest('theme/assets'));
    .pipe(tap(function(file){
      upload(file);
    }));
});

gulp.task('watch', function(){
  gulp.watch(paths.styles, ['sass']);
});

(example gulp-run log output)

$ theme upload assets/timber.scss.liquid
(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at WriteStream.EventEmitter.addListener (events.js:160:15)
    at WriteStream.Readable.on (_stream_readable.js:688:33)
    at WriteStream.EventEmitter.once (events.js:185:8)
    at Duplex.Readable.pipe (_stream_readable.js:540:8)
    at exec (/project/node_modules/gulp-run/gulp-run.js:90:17)
    at Transform.commandStream.exec (/project/node_modules/gulp-run/gulp-run.js:204:12)
    at upload (/project/Gulpfile.js:18:54)
    at /project/Gulpfile.js:39:7
    at Stream.modifyFile (/project/node_modules/gulp-tap/lib/tap.js:66:11)
    at Stream.stream.write (/project/node_modules/gulp-tap/node_modules/event-stream/node_modules/through/index.js:26:11)

Not sure if this is related to gulpjs/gulp#432 but seems to be coming from Transform.commandStream.exec in gulp-run. I'm running 0.10.28.

@cbarrick
Copy link
Collaborator

cbarrick commented Jul 9, 2014

The warning is "normal" and simply means you're processing more than 10 files in parallel. gulp-run spawns a new process to handle every file it receives as input and processes them in parallel. For each file, a logger object is created to handle piping the stdout of the child process to the stdout of the main process. Piping involves creating event listeners on process.stdout, hence the warning.

This log system could definitely be refactored to avoid this, but no memory is actually being leaked (as of 9303018), so you can safely increase the listener count on process.stdout to remove the warning.

Consider the following two examples that run test a = a one-hundred times.

Parallel Example

var count = 100;
function almostDone() {
    count -= 1;
    if (count === 0) done();
}

for (var i = count; i > 0; i -= 1) {
    run('test a = a').exec(almostDone);
}

Since exec is asynchronous, this example spawns 100 processes in parallel and calls done when they have all exited. You'll see the memory leak warning here because all 100 of the processes attach listeners to process.stdout. Since gulp pipelines are asynchronous, this is what happens and is usually what should happen from a performance standpoint.

Series Example

var count = 100;
function test() {
    run('test a = a').exec(function () {
        count -= 1;
        if (count > 0) test();
        else done();
    });
}
test();

This example also spawns 100 processes, but it waits for the previous to finish before spawning the next. In this case, you won't see the warning because each process releases it's listeners before the next process starts. And since the warning is not thrown, this example shows that each execution properly cleans up after itself.

The solution

As far as I can tell, the solution is process.stdout.setMaxListeners(n + 1) where n is the number of files being processed. Or just process.stdout.setMaxListeners(Infinity).

@cbarrick cbarrick closed this as completed Jul 9, 2014
@pushred
Copy link
Author

pushred commented Jul 10, 2014

Awesome, thanks so much for this detailed explanation. This all makes perfect sense. The concurrency is definitely desirable!

cbarrick added a commit that referenced this issue Jul 14, 2014
- The code is simpler and easier to read
- You can now give standard input to `run(...).exec([stdin], [callback])`
    as a String, Buffer, or Vinyl file
- A new `Command` class is exposed for running commands outside of gulp
- Fixes #8
- No need to set the max listeners on `process.stdout` (#7)
- Removed dependency on CircularBuffer
@cbarrick
Copy link
Collaborator

I recently published a rewrite that avoids the issue altogether. See v1.6.0 and the updated readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants