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

Remove all event listeners on close to avoid memory leak #808

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

jared-hexagon
Copy link
Contributor

Summary

Fix for #805 by removing all event listeners on prompt.ui.close().

Testing

In the repro (https://github.com/jared-hexagon/inquirer-memory-leak-repro) I npm linked my fork and the MaxListenersExceededWarning warning never shows up.

@SBoudrias
Copy link
Owner

I'm not too sure about this change.

The problem is that we'll end up removing ALL listeners; even those that might've been setup outside of the Inquirer code base.

I'd prefer to specifically remove the listeners we're adding.

@jared-hexagon
Copy link
Contributor Author

The problem is that we'll end up removing ALL listeners; even those that might've been setup outside of the Inquirer code base.

It only removes listeners for a single event emitter. If we create the emitter we should be safe to remove listeners for it (which we do in baseUI.js on line 15). Otherwise I think it will be a pain in the ass to remove each listener because of the layers going on.

@SBoudrias
Copy link
Owner

Yeah for sure won't be easy... Let's try this out, and if no one raises issues, we should be good to go.

@haoqunjiang
Copy link

#811
So this is causing bugs. Please revert.

@chrisdothtml
Copy link
Contributor

In light of #811 I looked into this, and from what I can tell, this.rl is shared across all prompts. The readline instance is initially setup in lib/ui/baseUI.js:15, and the same instance (this.rl) is passed into all subsequent prompt instances created at lib/ui/prompt.js:85. So calling removeAllListeners removes listeners for all future prompts as well as the instance being closed.

I'm not too sure about this change...
...

Let's try this out, and if no one raises issues, we should be good to go

@SBoudrias in retrospect, perhaps a bit more scrutiny should be put on changes like this considering this package has 12mil+ weekly downloads

@jared-hexagon
Copy link
Contributor Author

@chrisdothtml

big_yikes

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.

4 participants