Event listeners warning on multiple prompts

[mpal9000]

Hello, thanks for this nice module!

When running multiple prompts, I get this warning:
"(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit."

Tests:

  var inquirer = require('inquirer');
  var prompt1 = function (callback) {
    inquirer.prompt([{
      type: 'list',
      name: 'choice',
      message: 'Select:',
      choices: ['one', 'two']
    }], function () {
      callback(callback);
    });
  };
  var prompt2 = function () {
    inquirer.prompt([{
      type: 'list',
      name: 'choice',
      message: 'Select:',
      choices: ['three', 'four']
    }], function () {});
  };
  var prompt3 = function () {
    inquirer.prompt([{
      type: 'list',
      name: 'choice',
      message: 'Select:',
      choices: ['five', 'six']
    }], function () {});
  };


  // Test 1
  prompt1(prompt1);

  // Test 2
  for (var i = 1; i < 11; i += 1) { prompt2(); }

  // Test 3
  for (var i = 1; i < 11; i += 1) { if (i % 2 === 0) { prompt2(); } else { prompt3(); } }

[danielchatfield]

Technically that code is not compatible with inquirer as each subsequent prompt should be called from within the callback of the previous one (see examples/nested-call.js) (or use async to handle this for you), and I can't think of a case where you couldn't just use a single inquirer.prompt call with multiple choices (although there may be one).

Even if you remove the eventEmitter limit then it will be buggy as the keypress and SIGINT listeners would be added numerous times causing all sorts of problems.

But I think this is something inquirer should support (or if it doesn't then it should fail with a warning when prompt is called before the previous prompt call has terminated).

One way to support this is to have an inquirer.queue so when inquirer.prompt is called it adds an anonymous function (that runs the prompt) to the queue and then if inquirer isn't already running (inquirer.running is false) it calls the first function in the queue (which will be the one it just added) and sets inquirer.running to true. Then as part of the onCompletion function it pops itself from the queue and then checks whether the queue is empty - if it is then it sets running to false, if it is not empty then it runs the next prompt in the queue.

This will prevent multiple event listeners from being attached (whilst still maintaining the ability for tests to override the stream as the listeners will be attached and removed before and after each prompt in the queue).

@SBoudrias I'm bad at explaining things but I hope you got the gist of what I am saying, I'll knock up a pull-request when I get back later today for you to take a look at.

[danielchatfield]

@SBoudrias Is this still valid? The tests don't override the stream like that any more.

[mpal9000]

My use case is to maintain the flow, while navigating between different sections in a script. Something like an old school menu with "select" and "back" options.

So it's like "Test 1" (nested calls). I added "Test 2" and "Test 3", because I think a solution to "Test 1" will fix those too and a warning from Node should be prevented in any case (except misuse). Like with errors.

The example in ./examples/nested-calls.js is not good for testing nested calls, because there is only one nesting and the warning that this issue is about, is printed only when the nestings are more than nine (with the default "maxListeners" value). Nested calls should be infinite theoretically. Like in "Test 1".

[danielchatfield]

I completely overlooked your test1 and assumed that the problem was with 2 and 3 (didn't notice that test1 was infinite and thought it was just one call).

There are 2 bugs here:

There is a memory leak (I'll post more details on this below), which will cause all 3 test cases to fail. Even after fixing the memory leak test 2 and 3 will still fail as currently inquirer cannot be called like that - it is asynchronous so the subsequent calls to inquirer.prompt will happen before the first one has finished and then you get a mess of loads of event listeners all trying to do stuff at the same time.

[danielchatfield]

The memory leak is something to do with mute-stream, I'm not sure whether it's a bug with inquirer and we are not closing the mute-stream properly (the docs imply that this shouldn't be necessary and that by closing the readline the mute-stream will be automatically closed or somewhere else but I'm investigating now.

[mpal9000]

// Test 1 (visually better)

var inquirer = require('inquirer');

var nestingsCounter = 0;
var maxNestings = 10;

var promptsGroup = function (callback) {
  inquirer.prompt([{
    type: 'list',
    name: 'choice-A' + nestingsCounter,
    message: 'Select:',
    choices: [
      'value-A' + nestingsCounter + '.1',
      'value-A' + nestingsCounter + '.2'
    ]
  }, {
    type: 'list',
    name: 'choice-B' + nestingsCounter,
    message: 'Select:',
    choices: [
      'value-B' + nestingsCounter + '.1',
      'value-B' + nestingsCounter + '.2'
    ]
  }], function (answers) {
    console.log(
      'choice-A%d:',
      nestingsCounter,
      answers['choice-A' + nestingsCounter]
    );
    console.log(
      'choice-B%d:',
      nestingsCounter,
      answers['choice-B' + nestingsCounter]
    );
    console.log('');

    nestingsCounter += 1;

    if (nestingsCounter <= maxNestings) {
      callback(callback);
    } else {
      console.log('Maximum nested prompts number (%d) reached!', maxNestings);
    }
  });
};

promptsGroup(promptsGroup);

[danielchatfield]

The test that I am using at the moment is (no user input required):

var inquirer = require('inquirer');

var nestingsCounter = 0;
var maxNestings = 10;

var promptsGroup = function (callback) {
  inquirer.prompt([{
    type: 'input',
    name: 'option' ,
    message: 'Nest:',
    default: nestingsCounter
  }], function (answers) {

    nestingsCounter += 1;

    if (nestingsCounter <= maxNestings) {
      callback(callback);
    } else {
      console.log('Maximum nested prompts number (%d) reached!', maxNestings);
    }
  });
  console.log("");
  inquirer.rl.emit("line");
};

promptsGroup(promptsGroup);

[mpal9000]

Nice, so we have a test and an example!

[danielchatfield]

cleanup isn't being called. It is called automatically when:

• the end event is emitted on the source
• the close event is emitted on the source
• the close event is emitted on the destination

Since the destination is process.stdout we can't close that. We close the readline interface but this does not close the input or the output streams so cleanup is never fired.

To fix this leak we need to call mute-stream's end method.

By adding this.rl.output.end(); the above test doesn't fail (but it currently causes other tests to fail). I'm investigating the other tests now (at least one of them I can see is because we overwrite the output stream with something that doesn't have an end method).

[mpal9000]

It works fine now, thanks!

[SBoudrias]

Fix is out on release 0.3.2

Thank you for the report, and thanks to @danielchatfield for the quick fix!