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(Shard): EventEmitter listener warning #7240

Merged
merged 11 commits into from Jan 11, 2022
Merged

fix(Shard): EventEmitter listener warning #7240

merged 11 commits into from Jan 11, 2022

Conversation

legendhimself
Copy link
Contributor

@legendhimself legendhimself commented Jan 10, 2022

Please describe the changes this PR makes and why it should be merged:
Attaching a message listener to the parent and child during the broadcastEval and fetchClientVlaues throws a MaxListenersExceededWarning:
ErrorLog: https://sourceb.in/GBCHPCxLr5
The above changes were tested for 24 hours+ and it fixed the warning for all of my shards.

Status and versioning classification:
Code changes have been tested against the Discord API, or there are no code changes
This PR changes the library's interface (methods or parameters added)

@imranbarbhuiya
Copy link
Contributor

You need to update the typings too.

@meister03
Copy link
Contributor

The error has a reason. It warns you for avoiding memory leaks, when to many listeners are opened.
There could be unwanted issues, when you eval broadcastEvals, which are never resolved.
Besides that this is also done on collectors, maybe a timeout would be a fix for the upper issue. (+ your code)

Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

Please make these changes! There should be no spaces between | (consistency) for JSDoc & the tests will fail due to missing semicolons.

packages/discord.js/src/sharding/Shard.js Outdated Show resolved Hide resolved
packages/discord.js/src/sharding/Shard.js Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/src/sharding/ShardClientUtil.js Outdated Show resolved Hide resolved
packages/discord.js/src/sharding/ShardClientUtil.js Outdated Show resolved Hide resolved
legendhimself and others added 8 commits January 10, 2022 21:24
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
@iCrawl iCrawl changed the title fix(Shard): EventEmitter memory leak fix(Shard): EventEmitter listener warning Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants