Skip to content

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Apr 2, 2020

Step 2 for #5011

Increase number of process listeners to avoid warnings.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bajtos
Copy link
Member

bajtos commented Apr 2, 2020

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.

@boneskull
Copy link
Contributor

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)

@boneskull
Copy link
Contributor

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

@raymondfeng
Copy link
Contributor Author

Close to favor #5020

@raymondfeng raymondfeng closed this Apr 2, 2020
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