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(ShardingManager): client error event cannot be emitted #5559

Merged
merged 4 commits into from
Jun 7, 2021

Conversation

meister03
Copy link
Contributor

When a Error happens during sending, it can not overwrite err.message. So that a error happens: "TypeError: Cannot set property message of which has only a getter"

Please describe the changes this PR makes and why it should be merged:
The Error can be reproduced with having the Sharding Manager on Worker Mode and than broadcastEvaling this.guilds.cache,
the upper scenario works fine for child processes, bc they support many types, but worker threads does not support functions and some other types. So a error comes: DOMException [DataCloneError]: #<Object> could not be cloned.
This Error can just be logged out when you merge the commit, bc there is a error itself on the _respond function, which should be fixed with the commit. I checked it, multiply times...

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes

When a Error happens during sending, it can not overwrite err.message. So that a error happens: "TypeError: Cannot set property message of  which has only a getter"
@BannerBomb
Copy link
Contributor

I honestly don't agree with this fix. Because you are just creating an object with the key "err" with the value of the error class then adding a "message" key to it. Instead of emitting an error class to the error event (which is what it's supposed to receive), you are now emitting an object. The TypeError happens because the DOMException class has a read-only message property which might need some handling for that. With the # could not be cloned error, I don't think you can send collections/maps to workers. You would need to serialize it to a transferable data structure like an object.

@meister03
Copy link
Contributor Author

Should I do Object assign?
and copy the error on a new object?
because the err.message is not writeable

creating a new instance of err to overwrite err.message

Co-authored-by: monbrey <[email protected]>
Upgrade from `let` to `const`

Co-authored-by: Jan <[email protected]>
@kyranet kyranet requested a review from iCrawl May 7, 2021 16:22
@meister03 meister03 changed the title Update ShardClientUtil.js Sharding Manager: client error event cannot be emitted, bc of a bug in the code May 10, 2021
@kyranet
Copy link
Member

kyranet commented Jun 2, 2021

Any update on this @meister03? Vlad's request is still pending.

@meister03
Copy link
Contributor Author

meister03 commented Jun 2, 2021

Any update on this @meister03? Vlad's request is still pending.

@kyranet
Which update? If the code works?

@kyranet
Copy link
Member

kyranet commented Jun 2, 2021

Vlad left a requested change above, which is still left without a reply.

Okay tested it and It seems to work

Co-authored-by: Vlad Frangu <[email protected]>
@meister03
Copy link
Contributor Author

meister03 commented Jun 3, 2021

Vlad left a requested change above, which is still left without a reply.

Forgot to test it, and it works, commited the Suggestion

@kyranet kyranet requested a review from vladfrangu June 3, 2021 09:13
@iCrawl iCrawl changed the title Sharding Manager: client error event cannot be emitted, bc of a bug in the code fix(ShardingManager): client error event cannot be emitted Jun 7, 2021
@iCrawl iCrawl merged commit d1c5b6f into discordjs:master Jun 7, 2021
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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.

8 participants