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

Keypress listeners not removed on Windows #899

Closed
mszydlowski91 opened this issue Mar 8, 2020 · 7 comments · Fixed by #904
Closed

Keypress listeners not removed on Windows #899

mszydlowski91 opened this issue Mar 8, 2020 · 7 comments · Fixed by #904

Comments

@mszydlowski91
Copy link

Since in previous versions you had problems closing the readline interface, as it hanged Node event loop, you have worked the issue around by:

if (!/^win/i.test(process.platform)) { this.rl.close(); }

Which means that every time a prompt is created, a keypress listener will still remain, causing excession of max listeners and leading to memory leaks. Do you plan on fixing this rather dirty workaround? It makes the package either unusable or requiring heavy additional logic on Windows terminals.

@shirshak55
Copy link

shirshak55 commented Mar 8, 2020

@mszydlowski91 i noticed signit is also not working when i do ctrl+c. Is it due to this? and is there any alternative fix to this problem lately its giving lot of issue :(

@mszydlowski91
Copy link
Author

@mszydlowski91 i noticed signit is also not working when i do ctrl+c. Is it due to this? and is there any alternative to this package i need it only for choice but lately it seems to have so much problem :(

UI.close method is invoked upon prompt runner completion, so I think it might be the same issue, although I never tested your case. Does it happen only on Windows terminals? If it works fine e.g. on tty, then it's very likely it's the same root cause.

@shirshak55
Copy link

@mszydlowski91 it happens on gitbash which is window terminal only. On OSX it works fine.

@mszydlowski91
Copy link
Author

Yep, because the regex I pasted above only checks for win, so on OSX it works fine. Which makes sense, since Linux and OSX are basically the same core, so if rl.close doesn't hang event loop on Linux, then it likely works on all Unix systems.

@mshima
Copy link
Contributor

mshima commented Mar 8, 2020

This workaround is for a node bug:
nodejs/node#21771
But can be reverted if causes more problems then fixes.

@mszydlowski91
Copy link
Author

Well, it kind of makes things worse as it tricks people into thinking it works fine, and then only after more thorough testing it turns out it's a ticking bomb. Right now it just forces you to do another dirty workarounds

@shirshak55
Copy link

hmm old version didn't have this issue right?

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 a pull request may close this issue.

3 participants