chore(core): set max listeners to 256 for process to avoid warnings#5015
chore(core): set max listeners to 256 for process to avoid warnings#5015raymondfeng wants to merge 1 commit intomasterfrom
process to avoid warnings#5015Conversation
There was a problem hiding this comment.
I am concerned about increasing the global max-listener option. There is a good reason why Node.js reports this warning, because it's often a sign that the application has a leak. (Plus the performance of EventEmitter degrades for high number of listeners.)
Ideally, we should find a way how to fix Mocha so that it does not install so many listeners (/cc @boneskull).
If that's not possible, then let's rework this pull request to increase the global max listeners number only in our test suite.
(EDITED): The problem I see in your proposal is that it effectively disables max-listener warning for all LB4 applications.
|
When we disable max-listener warnings in our tests, then we won't notice if our code is leaking event listeners. IMO, we should avoid changing the global max-listener configuration at all costs and look for ways how to change max-listeners config in specific places (on specific event emitter instances) only. |
|
Which listeners do you mean? Mocha isn’t listening for SIGINT more than once or twice. I was thinking those came out of LB (or a dep) |
|
but fwiw I wouldn’t recommend bumping it either, unless we can show that it’s not a leak and they won’t grow further |
|
Close to favor #5020 |
Step 2 for #5011
Increase number of process listeners to avoid warnings.
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈