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

Fix bug to properly close readline. #52

Merged
merged 1 commit into from
Jul 28, 2013
Merged

Fix bug to properly close readline. #52

merged 1 commit into from
Jul 28, 2013

Conversation

danielchatfield
Copy link
Collaborator

@SBoudrias I have found the bug causing #49, turns out all the blocking code is already in place and working fine and it was this pesky code bug:

(Only showing problem code)

cli.prompt = function( questions, allDone ) {
  /* ... */
  this.rl || (this.rl = readlineFacade.createInterface());
  this.rl.resume();
 /* ... */
  var onCompletion = function() {
    /* ... */
    this.rl.close();
    this.rl = null;
    /* ... */
}
/* ... */
}

As you can see resume() is called on readline even though there is no pause implemented. In fact as it stands it is not possible for readline to be present since it always get's destroyed in onCompletion.

To fix this we should call this.rl.pause(); and remove the line that destroys the readline Interface.

@danielchatfield
Copy link
Collaborator Author

@SBoudrias I see that there is a test that inquirer should always create a new instance of readline, what is the reason behind this?

If there is a reason why this behaviour is necessary then I'll have to work on getting it to work with close() but I can't see any reason why it is not working with close() with the current code (I tried removing the resume call but that doesn't fix it) so I suspect it is a bug in node somewhere.

@SBoudrias
Copy link
Owner

The rl was paused initially, but this created issue when the prompt was nested (like the bug you bring up on win8).

The relevant commit: 1a7bc41
And I think this was the issue bringing this fix up: yeoman/yo#51

None the less, I'll test your current patch and see if the same issue happen again.

@danielchatfield
Copy link
Collaborator Author

If we do need to use close then another way of fixing it (although I don't have a clue why it is fixing it) is calling pause before close.

There is definitely a node bug and I've managed to reduce it down to a smallish testcase and opened a ticket: nodejs/node-v0.x-archive#5927

@SBoudrias
Copy link
Owner

If we do need to use close then another way of fixing it is calling pause before close.

If this works, let fix it this way.

@danielchatfield
Copy link
Collaborator Author

Ok, I'll update the pull request when I've finished my dinner.

@danielchatfield
Copy link
Collaborator Author

@SBoudrias I've updated the pull request.

SBoudrias added a commit that referenced this pull request Jul 28, 2013
Fix bug to properly close readline.
@SBoudrias SBoudrias merged commit c9302d3 into SBoudrias:master Jul 28, 2013
@SBoudrias
Copy link
Owner

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants